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

[Tooling] Update Ruby Dependencies #21129

Merged
merged 1 commit into from
Aug 12, 2024
Merged

Conversation

AliSoftware
Copy link
Contributor

@AliSoftware AliSoftware commented Aug 12, 2024

This should address the currently 3 opened Dependabot alerts about Ruby gems—in particular about rexml:

Git Oddity?!

@spencertransier already created and merged #21120 a couple of days ago, which was supposed to have solved those already.

Yet, looking at the current Git History of the Gemfile.lock from trunk, the merge commit for #21120 doesn't appear anywhere 🤔 😕

Especially even if the changes from #21120 had for some reason been reverted, or overwritten by a subsequent PR doing unrelated changes to Gemfile.lock and accidentally undoing the changes from https://github.com/wordpress-mobile/WordPress-Android/pull/21120… we'd also see that in the git history for the Gemfile.lock, right?

Anyway, not sure what happened here, but I figured it'd still make sense to re-do that gem update in any case.

@AliSoftware AliSoftware self-assigned this Aug 12, 2024
@AliSoftware AliSoftware added this to the 25.6 milestone Aug 12, 2024
@AliSoftware AliSoftware requested a review from a team August 12, 2024 11:10
Copy link

@wpmobilebot
Copy link
Contributor

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr21129-cdad9f8
Commitcdad9f8
Direct Downloadwordpress-prototype-build-pr21129-cdad9f8.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr21129-cdad9f8
Commitcdad9f8
Direct Downloadjetpack-prototype-build-pr21129-cdad9f8.apk
Note: Google Login is not supported on these builds.

@AliSoftware
Copy link
Contributor Author

AliSoftware commented Aug 12, 2024

Git Oddity?!

@spencertransier already created and merged #21120 a couple of days ago, which was supposed to have solved those already.

Yet, looking at the current Git History of the Gemfile.lock from trunk, the merge commit for #21120 doesn't appear anywhere 🤔 😕

Especially even if the changes from #21120 had for some reason been reverted, or overwritten by a subsequent PR doing unrelated changes to Gemfile.lock and accidentally undoing the changes from https://github.com/wordpress-mobile/WordPress-Android/pull/21120… we'd also see that in the git history for the Gemfile.lock, right?

Anyway, not sure what happened here, but I figured it'd still make sense to re-do that gem update in any case.

Uh, it seems that #21121 is the one that accidentally reverted changes to the Gemfile.lock and did set some Ruby gems versions back to earlier versions instead (e.g. reverted rexml from 3.3.4 to older version 3.2.9 that included the CVE vulnerability).

I think that happened because @nbradbury 's PR branch was initially created a while ago (and thus that branch had been cut before we updated the gems in #21120 and landed that one in trunk); or maybe this was some kind of race condition in Github. I'm also not sure why git and GitHub (1) didn't flag this accidental reversal a a git conflict, or (2) didn't auto-resolve this conflict of Gemfile.lock changes by picking the latest and more recent changes from trunk instead of picking the state of the Gemfile.lock from Nick's branch. I'd have expected Git to be smarter here and pick the one from trunk given its more recent.

And also, still not sure why this made Git and GitHub not show any changes in the Git History of the Gemfile.lock on trunk—as opposed to showing Spencer's merge commit changing it, and Nick's merge commit accidentally changing it back. Note that git log Gemfile.lock shows the same result as GitHub, namely not showing Spencer's merge commit at all nor which subsequent commit was the cause of the accidental reversal (which made getting to the bottom of this not as trivial as it should have). :git: :old-man-yells-at-cloud:


Anyway, glad I managed to get to the bottom of what happened, even if I'm not sure why git behaved that way. At least this confirms that this PR and re-applying the change to the Gemfile.lock is the right fix to apply.

@wzieba
Copy link
Contributor

wzieba commented Aug 12, 2024

I'd also expect GitHub to have a feature of "you're trying to merge a PR with reported security vulnerability" out of the box 😓 . Maybe we could consider integrating https://github.com/actions/dependency-review-action ?

@AliSoftware AliSoftware merged commit 7475ff9 into trunk Aug 12, 2024
22 of 23 checks passed
@AliSoftware AliSoftware deleted the tooling/update-ruby-dependencies branch August 12, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants