Add action to fetch latest version and build number #186

jspargo · 2020-01-12T14:13:42Z

Fixes #149

This PR adds the Fastlane actions appcenter_fetch_version_number to fetch the latest version number for a given app, which is returned as a string in the format major.minor.patch.build.

The action uses the App Center API to list releases, which is one of the 'distribute' endpoints. It uses the value received from the 'version' key, though I'd be interested to see if there's an appetite to provide an action/parameter to provide the value for 'short_version'.

Usage

This action can be used simply by calling appcenter_fetch_version_number, which returns the version number as a string. For example, if your Fastfile looked like this

lane :test
  puts appcenter_fetch_version_number
end

then provided the environment variable APPCENTER_API_TOKEN is set, the user will be prompted to choose their app from a list (if there is more than 1) when running this lane. If it's not set, then api_token should be passed as a parameter.

For CI and other non-interactive use cases, the app name should be specified as a minimum, however specifying the owner name too saves on an API call:

lane :test
  version = appcenter_fetch_version_number(
    api_token: "my-api-token",
    owner_name: "MyAppCenterAccountName",
    app_name: "MyApplication"
  )
  puts version
end

What about `latest_appcenter_build_number`?

To be clear, the functionality contained in this PR is identical to what currently exists in the fastlane-plugin-latest_appcenter_build_number plugin. If this PR is acceptable and the action within it are merged into this repository, I will put a notice in the README and a command-line-warning asking users to adopt this as the official App Center plugin and make it clear that additional features and bug fixes will only be addressed in releases from this repository.

…nter_build_number' plugin

msftclas · 2020-01-12T14:13:55Z

All CLA requirements met.

jp-andre

Hi! Thanks so much for contributing this PR!
It's really great to see a plugin author take interest in merging functionality directly into our plugin.

I've left a few comments in the code.
As for short_version it would be great to include it in this PR. How about returning an object that contains { version, build_number }? Note that the upload action uses version and build_number that somehow match the upload API call, but the release GET calls return version and short_version. For reference we have this conversion code in the upload action:

          params[:version] = release['short_version']
          params[:build_number] = release['version']

Suggestions:

Should we also include the release ID?
Should the upload action return the same values as this function?

Do the requested changes make sense to you?

jp-andre · 2020-01-13T07:07:35Z

lib/fastlane/plugin/appcenter/helper/appcenter_helper.rb

+
+      # Checks that the app name is valid
+      def self.check_valid_name(name)
+        regexp = /^[a-zA-Z0-9\-]+$/i


I think this regex is not entirely correct. We can create app names with . (dot) and _ (underscore). I'm not 100% sure if any other character is possible. Also note that the trailing i (for case insensitiveness) is redundant with a-zA-Z :)

This is the regex in the model: /^[a-zA-Z0-9-_.]+$/

jp-andre · 2020-01-13T07:08:36Z

lib/fastlane/plugin/appcenter/actions/appcenter_fetch_version_number.rb

+
+        unless app_name.nil?
+          unless Helper::AppcenterHelper.check_valid_name(app_name)
+            UI.user_error!("The `app_name` ('#{app_name}') cannot contains spaces and must only contain alpha numeric characters and dashes")


If the name is invalid, I would print a warning but not fail immediately. See my other comment about check_valid_name. In the end, if the HTTP calls pass, it means the name was valid :)
You might want to use optional_error for that (as seen in the upload action).

We probably can also mention in this warning message that the app_name is what appears in the app URL.

jp-andre · 2020-01-13T07:17:51Z

lib/fastlane/plugin/appcenter/actions/appcenter_fetch_version_number.rb

+          return nil
+        end
+
+        host_uri = URI.parse('https://api.appcenter.ms')


These HTTP calls should be moved to the helper class, and use the same patterns, if possible.

jp-andre · 2020-01-13T07:39:23Z

lib/fastlane/plugin/appcenter/actions/appcenter_fetch_version_number.rb

+
+      def self.get_owner_and_app_name(api_token)
+        apps = get_apps(api_token)
+        app_matches = prompt_for_apps(apps)


There can be only a single value here, right?

jp-andre · 2020-01-13T07:41:30Z

lib/fastlane/plugin/appcenter/actions/appcenter_fetch_version_number.rb

+
+      def self.prompt_for_apps(apps)
+        app_names = apps.map { |app| app['name'] }.sort
+        selected_app_name = UI.select("Select your project: ", app_names)


What is the behavior in non-interactive mode? We should require both owner_name and app_name to be set in that case.

jp-andre · 2020-01-13T07:44:20Z

lib/fastlane/plugin/appcenter/actions/appcenter_fetch_version_number.rb

+          owner_name = get_owner_name(api_token, app_name)
+        else
+          unless Helper::AppcenterHelper.check_valid_name(owner_name)
+            UI.user_error!("The `owner_name` ('#{owner_name}') cannot contains spaces and must only contain lowercased alpha numeric characters and dashes")


Same comment.

jp-andre · 2020-01-13T07:48:21Z

lib/fastlane/plugin/appcenter/actions/appcenter_fetch_version_number.rb

+        api_token = params[:api_token]
+        app_name = params[:app_name]
+        owner_name = params[:owner_name]
+        if app_name.nil? && owner_name.nil?


I think this case should not be allowed if we're running non-interactively (eg. CI build).

lib/fastlane/plugin/appcenter/actions/appcenter_fetch_version_number.rb

jspargo · 2020-01-14T21:25:15Z

Thanks @jp-andre! Just taking a look through your review, but at a glance I think it all makes sense. I might remove the valid name check altogether for the time-being - it could always be re-added across all actions in this plugin if that's something that you and the team think would be useful. What do you think?

I'm thinking perhaps the object returned could be {id, version, build_number }? I think ID would be useful. One thing I don't quite follow is that in your conversion code build_number is being set from version in the API response, but version is the full version number including the build number (the CFBundleVersion or versionCode) - so for:

        "id": 1,
        "short_version": "1.0.0",
        "version": "1.0.0.1234",

are you thinking that the object would be {1, "1.0.0", "1.0.0.1234" }?

jp-andre · 2020-01-15T01:46:58Z

Thanks @jp-andre! Just taking a look through your review, but at a glance I think it all makes sense. I might remove the valid name check altogether for the time-being - it could always be re-added across all actions in this plugin if that's something that you and the team think would be useful. What do you think?

Sounds good, feel free to adjust or remove as you prefer. (Note that . (dot) is an edge case and could be filtered out).

I'm thinking perhaps the object returned could be {id, version, build_number }? I think ID would be useful. One thing I don't quite follow is that in your conversion code build_number is being set from version in the API response, but version is the full version number including the build number (the CFBundleVersion or versionCode) - so for:
        "id": 1,
        "short_version": "1.0.0",
        "version": "1.0.0.1234",
are you thinking that the object would be {1, "1.0.0", "1.0.0.1234" }?

I believe Android's versionCode would be returned as version and that's what we would use as build_number. In other words, the API should return:
{ "id": 1, "short_version": "1.0.0", "version": "1234", ... }
And we would return { 1, "1.0.0", "1234" }, though in my opinion returning a hash here would be easier to understand:

{ "id" => 1, "version" => "1.0.0", "build_number" => "1234" }

I don't know any case where we build and return a "full version" string as you mentioned. But if it's happening, I'd love to know and look into it.

Apart from these comments, could you amend the README as well to add a mention to this new action?

jspargo · 2020-01-15T08:53:48Z

Thanks @jp-andre

I don't know any case where we build and return a "full version" string as you mentioned. But if it's happening, I'd love to know and look into it.

I primarily work with iOS apps and have always found version to return the CFBundleVersion and short_version to return CFBundleShortVersionString when using App Center upload (and back in HockeyApp days too), but perhaps this is a bug?

Personally, I’d prefer it if the API returned what you described, but I have a feeling this is a limitation based on the way Apple handles version numbers. I’d be happy to share an example via a private channel, if that helps.

Edit: I suppose it doesn’t matter too much, just so long as build_number returns whatever value is listed in the version - what we do now in most cases where I’ve used my plugin is extract whichever part of the version we require (e.g. sometimes just the major number, sometimes just the build number, sometimes all of it)

jp-andre · 2020-01-15T09:24:34Z

I primarily work with iOS apps and have always found version to return the CFBundleVersion and short_version to return CFBundleShortVersionString when using App Center upload (and back in HockeyApp days too), but perhaps this is a bug?

This sounds about right. Each value should correspond to exactly one entry in the build configuration files. The rest is just Apple's convention. :)

jspargo · 2020-01-28T23:21:08Z

Hi @jp-andre - I've made the changes suggested. Changes since your last review in summary:

Removed checking name validity (I may put a PR up for this separately in future, if we think it might be useful)
Moved HTTP calls to helper class and follow pattern used there
Updated README to include this new action
Removed the interactive element of prompting for owner_name and app_name - these are now required (though Fastlane will prompt if they're missing, so that's helpful). Officially we should say that all 3 params are required.
Changed the returned value to a hash in the format:

{ "id" => 1, "version" => "1.0.0", "build_number" => "1234" }

Looking forward to hearing back from you soon!

jp-andre

This looks really good. I'll get a second opinion from my team mates, but for me it's all good.

lib/fastlane/plugin/appcenter/actions/appcenter_fetch_version_number.rb

fastlane/Fastfile

lib/fastlane/plugin/appcenter/actions/appcenter_fetch_version_number.rb

anywherepilot

Excuse the comment spam. This looks good to me!

…hecks

…dating messaging in error to make this clearer

jspargo · 2020-01-29T09:44:39Z

Thanks @jp-andre and @anywherepilot for your reviews and helpful comments! I've addressed all the remaining comments I think.

Jack Spargo added 5 commits January 11, 2020 14:11

initial commit to move structture and helpers over from 'latest_appce…

2c06085

…nter_build_number' plugin

updating fetch_version_spec to match appcenter spec style

bb3f897

adding AppcenterFetchVersionNumber methods

8478d33

add test lane to Fastfile for appcenter_fetch_version_number

8fdfa7f

tweaks in preparation for PR

4be1c90

jspargo changed the title ~~Get build number~~ Add action to fetch latest version and build number Jan 12, 2020

phillipleblanc requested a review from jp-andre January 13, 2020 05:57

jp-andre suggested changes Jan 13, 2020

View reviewed changes

Jack Spargo added 6 commits January 28, 2020 21:49

don't check name validity

cb69eed

PR comments and simplification

7d679aa

update README with appcenter_fetch_version_number action

2018ace

make owner_name and app_name non-optional

7e600c2

fixing specs

e89120d

return hash of id, version, and build

e7ea6fa

removing some unused code

686d53b

jp-andre approved these changes Jan 29, 2020

View reviewed changes

lib/fastlane/plugin/appcenter/actions/appcenter_fetch_version_number.rb Show resolved Hide resolved

fastlane/Fastfile Outdated Show resolved Hide resolved

anywherepilot reviewed Jan 29, 2020

View reviewed changes

lib/fastlane/plugin/appcenter/actions/appcenter_fetch_version_number.rb Outdated Show resolved Hide resolved

anywherepilot reviewed Jan 29, 2020

View reviewed changes

lib/fastlane/plugin/appcenter/actions/appcenter_fetch_version_number.rb Outdated Show resolved Hide resolved

anywherepilot reviewed Jan 29, 2020

View reviewed changes

lib/fastlane/plugin/appcenter/actions/appcenter_fetch_version_number.rb Outdated Show resolved Hide resolved

anywherepilot approved these changes Jan 29, 2020

View reviewed changes

Jack Spargo added 3 commits January 29, 2020 09:11

rubocop fixes

aa2a321

Updating README with response example and removing unused nil owner c…

677fe61

…hecks

Changing variable name to more accurate name: latest_release. Also up…

8716ad8

…dating messaging in error to make this clearer

jp-andre merged commit f11ee6c into microsoft:master Feb 3, 2020

jspargo deleted the get-build-number branch February 5, 2020 11:07

jspargo mentioned this pull request Nov 11, 2020

"release_notes", appcenter_fetch_app and appcenter_create_app #210

Closed

Add action to fetch latest version and build number #186

Add action to fetch latest version and build number #186

Uh oh!

Conversation

jspargo commented Jan 12, 2020

Usage

What about latest_appcenter_build_number?

Uh oh!

msftclas commented Jan 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jp-andre left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jspargo commented Jan 14, 2020

Uh oh!

jp-andre commented Jan 15, 2020

Uh oh!

jspargo commented Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jp-andre commented Jan 15, 2020

Uh oh!

jspargo commented Jan 28, 2020

Uh oh!

jp-andre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anywherepilot left a comment

Choose a reason for hiding this comment

Uh oh!

jspargo commented Jan 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

What about `latest_appcenter_build_number`?

msftclas commented Jan 12, 2020 •

edited

Loading

jspargo commented Jan 15, 2020 •

edited

Loading