Skip to content
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

Grab iOS version from release event #4957

Merged
merged 3 commits into from
Aug 31, 2021
Merged

Grab iOS version from release event #4957

merged 3 commits into from
Aug 31, 2021

Conversation

AndrewGable
Copy link
Contributor

Details

Uses the release event's tag to send to fastlane, to determine which version to promote to iOS production. It looks like the old ios.yml used to rely on bumpVersion.js to set this in the env which we cannot rely on anymore

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/175849

Tests

  1. I tested this syntax here: https://github.com/Andrew-Test-Org/Public-Test-Repo/runs/3476298417?check_suite_focus=true
  2. In order to fully test this, we will need to merge this and wait until we fully deploy a checklist

@AndrewGable AndrewGable self-assigned this Aug 31, 2021
@AndrewGable AndrewGable requested a review from a team as a code owner August 31, 2021 18:28
@MelvinBot MelvinBot requested review from robertjchen and removed request for a team August 31, 2021 18:29
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

Nice, I have one question and two comments:

  • Do we need to set the iOS version conditionally? i.e: can we set this environment variable for both prod and non-prod builds?
  • Let's use fromJSON instead of string comparison
  • Also, let's add a second line to the new step to just echo the IOS_VERSION environment variable to the console. Seems like useful debugging information.

.github/workflows/platformDeploy.yml Outdated Show resolved Hide resolved
@roryabraham
Copy link
Contributor

Also, should we get rid of the old code in bumpVersion.js that we once relied on to set the environment variable? Since then, we've kinda determined that relying on side-effects in actions is a bad practice.

AndrewGable and others added 2 commits August 31, 2021 11:56
Co-authored-by: Rory Abraham <47436092+roryabraham@users.noreply.github.com>
@AndrewGable
Copy link
Contributor Author

Do we need to set the iOS version conditionally? i.e: can we set this environment variable for both prod and non-prod builds?

We can, but it's not needed for staging as the version comes from elsewhere and additionally the release event won't be set

Let's use fromJSON instead of string comparison

Committed your suggestion!

Also, let's add a second line to the new step to just echo the IOS_VERSION environment variable to the console. Seems like useful debugging information.

👍

Also, should we get rid of the old code in bumpVersion.js that we once relied on to set the environment variable? Since then, we've kinda determined that relying on side-effects in actions is a bad practice.

We can remove the code in bumpVersion.js that sets the environment variable, but I don't think there is code that relies on it being set in JS. The only code that relied on it being set was the workflow that I am editing.

@roryabraham roryabraham merged commit 2919b88 into main Aug 31, 2021
@roryabraham roryabraham deleted the andrew-fix-ios-prod branch August 31, 2021 20:47
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

OSBotify commented Sep 1, 2021

🚀 Deployed to staging by @roryabraham in version: 1.0.90-3 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

OSBotify commented Sep 2, 2021

🚀 Deployed to production by @roryabraham in version: 1.0.91-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

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.

3 participants