woocommerce / woocommerce-android

WooCommerce Android app
https://www.woocommerce.com/mobile
GNU General Public License v2.0
277 stars 135 forks source link

DO NOT MERGE: Dummy change to trigger Dangermattic #12866

Closed AliSoftware closed 1 week ago

AliSoftware commented 2 weeks ago

This is a dummy PR for testing https://github.com/woocommerce/woocommerce-android/pull/12859

The below is dummy description extracted from PR https://github.com/woocommerce/woocommerce-ios/pull/14265 that contained videos that were not detected as such by the older Dangermattic version ### Typing invalid value https://github.com/user-attachments/assets/8cdc7b5a-9a88-423d-bf7b-f2cf872824a6 ### Scanning invalid value https://github.com/user-attachments/assets/f3461fec-f96c-4376-9e00-f8faf65f3457
dangermattic commented 2 weeks ago
1 Message
:book: This PR is still a Draft: some checks will be skipped.

Generated by :no_entry_sign: Danger

AliSoftware commented 2 weeks ago

@iangmaia any idea why the bug that happened on WCiOS 14265 didn't happen here?

I'd thus expect the run of Dangermattic in its old version to have the same issue as with WCiOS 14265, and only have it disappear if I rebased this draft PR on top of your https://github.com/woocommerce/woocommerce-android/pull/12859 to get the fix…?

wpmobilebot commented 2 weeks ago
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App Name WooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commit88aed22ad0ecea945e6776118b9f1e8ada6b0742
Direct Downloadwoocommerce-wear-prototype-build-pr12866-88aed22.apk
wpmobilebot commented 2 weeks ago

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App Name WooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commit88aed22ad0ecea945e6776118b9f1e8ada6b0742
Direct Downloadwoocommerce-prototype-build-pr12866-88aed22.apk
iangmaia commented 2 weeks ago

@iangmaia any idea why the bug that happened on WCiOS 14265 didn't happen here?

  • I've made the PR make changes to a *View.kt file
  • The PR branch is cut from trunk for the time being, meaning it doesn't yet have your fix from [Tooling] Update Dangermattic #12859
  • The view_changes_checker.check call in the Dangerfile is before the line that skips the rest of the checks if github.pr_draft?, so it should have been executed despite this PR being draft

I'd thus expect the run of Dangermattic in its old version to have the same issue as with WCiOS 14265, and only have it disappear if I rebased this draft PR on top of your #12859 to get the fix…?

Oof, I think I know what's happening.

When running the Danger task, we first make sure Danger and Dangermattic are installed:

gem install danger -v 9.5.0 --conservative
gem install danger-dangermattic -v 1.1.2 --conservative

And then we run Danger on that specific version... but I believe the Dangermattic version will just be the latest one installed?

danger _9.5.0_ --fail-on-errors\=true --danger_id\=pr-check

So once a newer Dangermattic version has been installed, it seems it will always be used? 🤔

AliSoftware commented 2 weeks ago

Oof, I think I know what's happening.

When running the Danger task, we first make sure Danger and Dangermattic are installed:

gem install danger -v 9.5.0 --conservative
gem install danger-dangermattic -v 1.1.2 --conservative

And then we run Danger on that specific version... but I believe the Dangermattic version will just be the latest one installed?

danger _9.5.0_ --fail-on-errors\=true --danger_id\=pr-check

So once a newer Dangermattic version has been installed, it seems it will always be used? 🤔

So you mean that's the same root as the bug that was discussed recently in p1730741535335339/1730740658.524069-slack-C02KLTL3MKM about Dangermattic using the latest version or rubocop installed in the linter agent? Interesting…

I wonder if a potential fix in Dangermattic could be to dynamically create an ad-hoc binstub/wrapper script using bundler/inline or even just an ad-hoc Gemfile that contains only the strictly necessary gems, to still ensure we only rely on a minimalistic set of gems being used (security) for the given danger job, all while still taking advantage of bundler to ensure we run the right version of the tool (instead of relying on the shady _#{version}_ trick that might apparently not work in / account for all cases or might not solve dependent gems as expected…)

iangmaia commented 2 weeks ago

So you mean that's the same root as the bug that was discussed recently in p1730741535335339/1730740658.524069-slack-C02KLTL3MKM about Dangermattic using the latest version or rubocop installed in the linter agent? Interesting…

It's slightly different, as in this case with Rubocop it is a bit easier to workaround as Danger runs Rubocop using a command that we can change, thus "force" a version. When we're talking about runtime dependencies, I think it becomes a bit more complicated.

iangmaia commented 2 weeks ago

or even just an ad-hoc Gemfile that contains only the strictly necessary gems, to still ensure we only rely on a minimalistic set of gems being used (security) for the given danger job, all while still taking advantage of bundler to ensure we run the right version of the tool

Yep, I think this can be a good idea 🤔 and it could also solve the Rubocop issue without having to resort to change the Rubocop command to something like rubocop _version_ 🤔