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

Bump rmagick to 5.3.0 #21008

Closed
wants to merge 1 commit into from
Closed

Bump rmagick to 5.3.0 #21008

wants to merge 1 commit into from

Conversation

wzieba
Copy link
Contributor

@wzieba wzieba commented Jun 19, 2024

Update rmagick to address https://github.com/wordpress-mobile/WordPress-Android/security/dependabot/24

Testing

I've tried to test it with fastlane android screenshots app:wordpress but the operation fails with junit.framework.AssertionFailedError: Unable to continue – expectation wasn't satisfied quickly enough. It's not related to rmagick (this fails also on trunk).

So I don't know how to test it, do you have an idea?

@wzieba wzieba requested review from AliSoftware and iangmaia June 19, 2024 12:42
@wzieba wzieba marked this pull request as ready for review June 19, 2024 12:42
@dangermattic
Copy link
Collaborator

dangermattic commented Jun 19, 2024

1 Warning
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@wzieba wzieba added the Tooling label Jun 19, 2024
Copy link

sonarcloud bot commented Jun 19, 2024

@AliSoftware
Copy link
Contributor

Yeah our screenshots automation is reguarly broken because no one maintains it and it's not run frequently enough (like, we update the screenshots once a year if not less… and even when we do somethings the Design team just generates them for all locales via Sketch manually instead so we don't even use the automation to create the screenshots from the UI tests…)

So short of fixing the automation (which would be a big project on its own), I'm not sure how we can properly test this.

The only thing I'd suggest checking is:

  • What are the breaking changes between major versions 4 and 5 or RMagick
  • If RMagick 5 requires a different version of the imagemagick binary (installed via brew), update our documentation accordingly if necessary (I'm not sure it will be needed, as at a glance we don't specify in that doc an explicit version of imagemagick to install… but worth double-checking)

In any case, given this fixes a security issue, and that if we want to use the automation to create screenshots we'll need to revisit the automation code and screenshots lanes that uses rmagick anyway before we can use it (and could then fix any major API breaking changes only then), I'd say better land this and fix the security issue and fix our screenshots code using it only later (maybe never…)

@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
Versionpr21008-04bec8e
Commit04bec8e
Direct Downloadwordpress-prototype-build-pr21008-04bec8e.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
Versionpr21008-04bec8e
Commit04bec8e
Direct Downloadjetpack-prototype-build-pr21008-04bec8e.apk
Note: Google Login is not supported on these builds.

@wzieba
Copy link
Contributor Author

wzieba commented Jun 19, 2024

Thank you for all the context @AliSoftware 🙏 I was confused how and when this is executed.

it's not run frequently enough (like, we update the screenshots once a year if not less…

I see - this is not very encouraging to start working on fixing the lane. It sounds like high effort / low gain work, at least at this moment.


I read more about the security vulnerability, and I came to a conclusion that I'll just close it, as it's about a possible DOS attack - something we're not prone to in the context of build tools for a mobile app.

Sorry for taking your time 😅

@wzieba wzieba closed this Jun 19, 2024
@AliSoftware
Copy link
Contributor

I see - this is not very encouraging to start working on fixing the lane. It sounds like high effort / low gain work, at least at this moment.

Yep. I remember already spending a lot of time around 2 years ago (see paaHJt-3wL-p2) trying to fix this, and got to a point where they were nearly working again, with just a small set of limitations and leftovers to fix… but I don't think we've used the lane since I fixed it, so now it (mostly the UI Tests that this lane uses to pilot the app's UI and take the right screenshots) has likely already drifted away again even more as the UI of the app has evolved more while that UI Test was never really maintained in sync.

I read more about the security vulnerability, and I came to a conclusion that I'll just close it, as it's about a possible DOS attack - something we're not prone to in the context of build tools for a mobile app.

Sounds good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants