-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ios_get_build_version action #256
Conversation
def self.run(params) | ||
require_relative '../../helper/ios/ios_version_helper.rb' | ||
|
||
UI.user_error!('You need to set at least the PUBLIC_CONFIG_FILE env var to the path to the public xcconfig file') unless ENV['PUBLIC_CONFIG_FILE'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to an implementation details of the ios_version_helper
– basically both get_internal_version
and get_build_version
rely on the private get_version_strings
method which require PUBLIC_CONFIG_FILE
to be set and returns the pair of public+internal, even if we don't end up using the public value from the returned tuple – the PUBLIC_CONFIG_FILE
env var has to be set even if we only want the internal version.
I considered this was not worth refactoring the whole version helper just to fix this, especially since PUBLIC_CONFIG_FILE
is always set in our use case and the context of all our host apps' Fastfile
anyway. We might get that cleaned up and that useless requirement lifted when we get to implement #203 later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may not be compatible with Jetpack, where it needs to share a repo (and fastfile) with WPiOS.
Could we accept it as a parameter, but fall back to ENV['PUBLIC_CONFIG_FILE']
if it's not present?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll get on that. Though note that many actions rely on such an env var, not just get_*_version
but also the various bump_*_version
ones, and same story for Android. So I wonder if that should be part of this PR vs having a dedicated PR to support Jetpack for all the iOS and Android actions related to version wrangling?
I'll make the change for ios_get_build_version
and take the occasion to replicate it on ios_get_app_version
but I'll let any other changes required in other actions and for all the platforms for a separate PR dedicated to Jetpack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, after trying to play around this, even trying to refactor just around ios_get_build_version
and nothing else quickly leads me down the rabbit hole, having to change get_versions()
which both methods rely on, but also having to update all the other methods relying on that private method and using those ENV
vars in the helper, including the ones that deal with bumping the version… And all that ends up being way more than just a couple of lines to change after all, and would require a dedicated pass of testing all the changes.
After chatting in Slack, the consensus is to keep that for a future and separate PR that would address all things related to Jetpack versioning, which means the whole family of {ios,android}_get_{app,build,alpha,release}_version
+ {ios,android}_{codefreeze,betabuild,hotfix,finalize}_prechecks
+ {ios,android}_bump_version_{beta,final,hotfix,release}
actions and all the helper methods they rely on.
Maybe when we get to address that – in a separate PR – that would be a good occasion to jump back on implementing #203 and continue the work I started in the 203-version
branch, which would help a lot in our goal to support multiple apps in a single repo (like WP+Jetpack) by stopping relying on ENV vars, global constants and methods, and same parameters having to be passed over and again to each class method calls, replacing all that with the passing of those parameters only to the constructor, then deal with the instance transparently from there.
TL;DR I won't implement that change in this PR after all, and we should have a separate and dedicated PR to add Jetpack support to all version methods, which extends way further than just amending this new action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkmassel given ☝️ , can you dismiss your "requested-changes" and give me a 👍 on the PR? 🥺
def self.run(params) | ||
require_relative '../../helper/ios/ios_version_helper.rb' | ||
|
||
UI.user_error!('You need to set at least the PUBLIC_CONFIG_FILE env var to the path to the public xcconfig file') unless ENV['PUBLIC_CONFIG_FILE'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may not be compatible with Jetpack, where it needs to share a repo (and fastfile) with WPiOS.
Could we accept it as a parameter, but fall back to ENV['PUBLIC_CONFIG_FILE']
if it's not present?
So that we can use it for the GH release and tag when creating betas
7f02480
to
abfd65d
Compare
Why
When creating a GitHub release (after a release build is triggered via
new_beta_release
orfinalize_release
in the host apps), iOS currently always usesios_get_app_version()
in the host Fastfiles as the version to use for the GitHub release title and tag name.This means that even GH releases created as a result of
new_beta_release
end up with a title and git tag ofx.y
instead of usingx.y.0.z
for betas.To solve this, the host app's
Fastfiles
will need to be able to fetch the build version (VERSION_LONG
inxcconfig
files) in order to pass it at call site tocreate_gh_release
and for it to use the correct title and tag for betas. This is what this PR adds.Target branch
This PR sits on top of
gh-releases/fix-target
from #253 because I needed both fixes at once to continue working on the fixes needed in the host apps'Fastfile
and the overall issues with GH releases in our process. #253 Should thus ideally be merged first – which should update this PR's branch todevelop
before we can merge that one next.To test
fastlane/Pluginfile
of an host app to point the release toolkit to this branch, and runbundle install
bundle exec fastlane run ios_get_build_version
. Notice it fails telling you an env var is missingPUBLIC_CONFIG_FILE=config/Version.public.xcconfig bundle exec fastlane run ios_get_build_version
and check that it returns the public build version (e.g.17.3.0.1
)PUBLIC_CONFIG_FILE=config/Version.public.xcconfig bundle exec fastlane run ios_get_build_version internal:true
, Notice it fails telling you an env var is missing.INTERNAL_CONFIG_FILE=config/Version.internal.xcconfig PUBLIC_CONFIG_FILE=config/Version.public.xcconfig bundle exec fastlane run ios_get_build_version internal:true
and check that it returns the internal build version (e.g.17.3.0.20210507
)