Skip to content

Conversation

@iangmaia
Copy link
Contributor

@iangmaia iangmaia commented Jan 27, 2023

What does it do?

This PR fixes #380 by adding a configuration item in the action ios_get_app_version, to be used instead of the environment variable ENV['PUBLIC_CONFIG_FILE'].

1️⃣ Adds the parameter public_version_xcconfig_file as a ConfigItem to the action ios_get_app_version.

The new parameter will be used to extract the app version, without relying on environment variables and deprecating the method that still does.
In order to to that, I've created a parameterised copy of the method Ios::VersionHelper.get_public_version(), which I called Ios::VersionHelper.get_xcconfig_public_version, also marking the original version as deprecated, and eventually we should stop using it.

Please note that there are still many uses in the codebase of the ENV['PUBLIC_CONFIG_FILE'] (as well as ENV['INTERNAL_CONFIG_FILE']), not only directly in the actions but by their dependencies. These should be addressed in follow up Pull Requests.

2️⃣ Adds basic tests to ios_get_app_version

Basic tests to the action functionality. These tests are gonna be extended in the next PR, where I'll also improve some of the related code in Ios::VersionHelper.

Related PRs

  • This PR improves the code for the .xcconfig parsing and adds new tests to ios_get_app_version

Checklist before requesting a review

  • Run bundle exec rubocop to test for code style violations and recommendations
  • Add Unit Tests (aka specs/*_spec.rb) if applicable
  • Run bundle exec rspec to run the whole test suite and ensure all your tests pass
  • Make sure you added an entry in the CHANGELOG.md file to describe your changes under the approprioate existing ### subsection of the existing ## Trunk section.

@AliSoftware AliSoftware self-requested a review January 30, 2023 18:45
@AliSoftware AliSoftware self-requested a review January 30, 2023 18:54
@iangmaia iangmaia force-pushed the add/xconfig-param-on-ios_get_app_version branch from e9ad618 to 1b6f330 Compare January 31, 2023 13:00
Copy link
Contributor

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, assuming we land iangmaia#1 on top of it as planned, to refine/improve the way the xcconfig file is read in the way you did in that other PR)

⚠️ Note that I'm still waiting for us to land my unrelated PR #449 in trunk and do a new release before starting to land your PRs, because I'm hoping we can make an initial (almost-)non-breaking release of the toolkit first, before starting to land your breaking-change ones.

@AliSoftware
Copy link
Contributor

⚠️ Note that I'm still waiting for us to land my unrelated PR #449 in trunk and do a new release before starting to land your PRs, because I'm hoping we can make an initial (almost-)non-breaking release of the toolkit first, before starting to land your breaking-change ones.

Actually, I just created and pushed a new branch called iangmaia/trial, currently pointing to the exact same commit as our current trunk… so that if you re-target your PR here to this iangmaia/trial branch instead of trunk, we could start landing it (and then other PRs of yours) there, and move forward with iangmaia#1 and all the other PRs you created on top of those… without us having to wait for me to land my PR and do a release first. I don't know why I didn't think of this before 😅

@iangmaia iangmaia changed the base branch from trunk to iangmaia/trial February 10, 2023 17:49
@AliSoftware
Copy link
Contributor

Dammit, now we have a conflict on the CHANGELOG.md that will need fixing before I can re-trigger CI and we can finally merge 😅 🙂

@iangmaia iangmaia force-pushed the add/xconfig-param-on-ios_get_app_version branch from 9de38e1 to fce43b3 Compare February 10, 2023 18:59
@iangmaia
Copy link
Contributor Author

Dammit, now we have a conflict on the CHANGELOG.md that will need fixing before I can re-trigger CI and we can finally merge 😅 🙂

Done!

@AliSoftware AliSoftware merged commit 5ca4081 into wordpress-mobile:iangmaia/trial Feb 10, 2023
@AliSoftware
Copy link
Contributor

CI took its time (Friday evening UTC is usually quite a busy period 😅 ) but finally went thru, all green and merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add configuration item for xcconfig with version info to ios_get_app_version

3 participants