-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Remove no-longer-required brew update
from CI commands
#20316
Conversation
We added this back in the day to address a Bintray issue that prevented Homebrew from installing dependencies. See 3a1c00c and #16373 The fix hasn't been required for a long time but the operation never gave us any trouble until some weeks ago, see 78fcc6f. Now it's a good time to go back and cleanup.
# Sentry CLI needs to be up-to-date | ||
brew upgrade sentry-cli |
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.
While self-reviewing the code, I realized this comment is not clear enough.
First, we need the CLI installed, and our lanes will fail if there isn't because the call sentry_check_cli_installed
, e.g.:
WordPress-iOS/fastlane/lanes/build.rb
Line 120 in 1be5d8c
sentry_check_cli_installed |
But, the sentry_check_cli_installed
action doesn't necessarily fails if the CLI is not at the latest version, only if it isn't at a version compatible with the Fastlane plugin where the action comes from. See the action description:
This action checks that the senty-cli is installed and meets the mimum verson requirements.
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.
Thinking about it, given the minimum CLI version is determined by what Sentry bundles in its Fastlane plugin the only instance in which this might fail is if we upgrade the plugin to a version that requires a new CLI. But, since we are in charge of when to upgrade the plugin, we can prevent or correct this failure and wait to upgrade till a new VM image is available. Alternatively, we can add the brew upgrade sentry-cli
command if we absolutely need the new Sentry plugin version and make a note to remove once a new VM image is out.
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.
Interestingly, the latest CLI version is 2.13.0 but the brew upgrade sentry-cli
command in CI doesn't find it 🤔
If I run brew info sentry-cli
locally, I get 2.13.0 as the latest, as one would expect.
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 may open a followup PR to either update the wording of that comment, or remove the whole Sentry CLI management based on the rationale from the previous comment.
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.
but the brew upgrade sentry-cli command in CI doesn't find it
Maybe we need to call brew update
before brew upgrade sentry-cli
, so it fetches the latest formulae first? 🤔
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.
@AliSoftware Good point. The logs I linked came from a release build, so it already run with brew update
commented...
Indeed, in builds that run brew update
the Sentry CLI updates as expected
Notice that my first comment said the latest Sentry CLI version was 2.13.0, but this screenshot shows 2.14.4. What I should have written originally is "the latest version bundled in the latest Fastlane plugin is 2.13.0".
📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
|
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.
🧹 🧹 🧹 ✨
# Sentry CLI needs to be up-to-date | ||
brew upgrade sentry-cli |
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.
but the brew upgrade sentry-cli command in CI doesn't find it
Maybe we need to call brew update
before brew upgrade sentry-cli
, so it fetches the latest formulae first? 🤔
@@ -1,7 +1,6 @@ | |||
#!/bin/bash -eu | |||
|
|||
# FIXIT-13.1: Prototype Builds want the latest version of Sentry CLI |
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.
Random question: does this "FIXIT-13.1" comment mean something like "this issue will be fixed in 13.1 version of the VM image"?
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 think so... It comes from a commit that explicitly mentions having issues in Xcode 13.
We added this back in the day to address a Bintray issue that prevented Homebrew from installing dependencies. See 3a1c00c and #16373
The fix hasn't been required for a long time but the operation never gave us any trouble until some weeks ago, see 78fcc6f.
Now it's a good time to go back and cleanup.
Testing
If CI is successful, in particular the installable builds, this change will be safe to merge.
Regression Notes
RELEASE-NOTES.txt
if necessary. N.A.