[HACK] A new CI step to check if dependencies were changed by kidinov · Pull Request #5420 · woocommerce/woocommerce-android

kidinov · 2021-12-08T12:20:41Z

Description

The PR adds a new CI step that checks if any dependencies (including transitive) were changed and makes a comment on the PR with a list of the dependencies

More details will be published tomorrow in a P2

Testing instructions

Try to add/remove/change the version of any dependency in the project
Push the changes
Wait for CI and notice a comment in this PR
Revert your change
Notice that comment is removed

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

peril-woocommerce · 2021-12-08T12:31:01Z

You can test the changes on this Pull Request by downloading the APK here.

AliSoftware

Overall I like the idea. I left a couple of nits/suggestions and questions.

But most importantly, I wonder if this PR shouldn't better wait for the migration of the repo to Buildkite, especially since multiple changes would be needed on those scripts after that migration anyway, and the script code could be simplified on multiple places thanks to Buildkite env vars and commands too…

PS: Do you happen to have a dummy draft PR built on top of this which would add a change in dependency and thus demonstrate the comment that would get posted in such cases? Would be nice to see it in action in a test PR before fully approving anyway 😜

AliSoftware · 2021-12-08T13:06:29Z

tools/dependency-tree-diff/dependency-tree-diff.sh

+    echo "There are no changes in dependencies of the project"
+    rm "$DIFF_DEPENDENCIES_FILE"
+  fi
+  ./gradlew dependencyTreeDiffCommentToGitHub -DGITHUB_PULLREQUESTID="${CIRCLE_PULL_REQUEST##*/}" -DGITHUB_OAUTH2TOKEN="$GITHUB_API_TOKEN" --info


@jkmassel Curious about your thoughts about this:

This script will require a couple of modifications when we will migrate the repository to Buildkite (hopefully soon), as we'd need to update CircleCI-specific env vars used (like CIRCLE_PULL_REQUEST, #CIRCLE_PROJECT_USERNAME, etc)

Once we migrate to Buildkite, it might not be worth anymore using yet another way to comment on a PR (i.e. using gradle here) compared to using your new comment_on_pr command from our buildkite plugin

Though both those changes (updating the env vars used + using comment_on_pr), as well as the simplifications that would be made possible in the script (see below)… would require this PR to wait for the migration to Buildkite to be done before going forward.

Anyway, I'm not sure where you are on your timing of migrating the client app repos to Buildkite… but given we very recently merged the comment_on_pr action which allowed you to start moving forward on this), I figured maybe you planned to start the migration of WCiOS very soon… which means it would be worth waiting for it before landing this PR (with the corresponding changes to make it work in Buildkite)?

It looks like that @jkmassel is afk. Let's wait then, I think it's not urgent anyway. Ill keep the PR open

AliSoftware · 2021-12-08T13:09:10Z

tools/dependency-tree-diff/dependency-tree-diff.sh

+  githubResponse="$(curl "$githubUrl" -H "Authorization: token $GITHUB_API_TOKEN")"
+  targetBranch=$(echo "$githubResponse" | tr '\r\n' ' ' | ./jq '.base.ref' | tr -d '"')
+
+  mkdir -p $DIFF_DEPENDENCIES_FOLDER


Quote the var to avoid issues with spaces in path

In general, I'd suggest you to run shellcheck on your .sh file to get a couple of suggestions to lint this shell script and fix things like this that are otherwise easy to miss 😉

Interestingly that spellcheck doesn't complain about this line. I quoted anyway though 3f0cc38

AliSoftware · 2021-12-08T13:10:53Z

tools/dependency-tree-diff/dependency-tree-diff.sh

+  git checkout "$CIRCLE_BRANCH"
+  ./gradlew :WooCommerce:dependencies --configuration $CONFIGURATION >$CURRENT_TARGET_BRANCH_DEPENDENCIES_FILE
+
+  ./tools/dependency-tree-diff/dependency-tree-diff.jar $TARGET_BRANCH_DEPENDENCIES_FILE $CURRENT_TARGET_BRANCH_DEPENDENCIES_FILE >$DIFF_DEPENDENCIES_FILE


Do we have the source for this JAR? Just so we could understand what it is supposed to do (e.g. what more does it do that a regular diff between the two .txt files wouldn't? Does it mine some Bitcoin behind our backs? What's the license for it?)

We do! https://github.com/JakeWharton/dependency-tree-diff

Nice! Worth a comment above that line in the script to mention that URL for future us IMHO 🙂

Good idea! Done - b941d24

tools/dependency-tree-diff/dependency-tree-diff.sh

AliSoftware · 2021-12-08T13:17:36Z

tools/dependency-tree-diff/dependency-tree-diff.sh

+  prNumber=$(echo "$CIRCLE_PULL_REQUEST" | sed "s/^.*\/\([0-9]*$\)/\1/")
+  githubUrl="https://api.github.com/repos/$CIRCLE_PROJECT_USERNAME/$CIRCLE_PROJECT_REPONAME/pulls/$prNumber"
+  githubResponse="$(curl "$githubUrl" -H "Authorization: token $GITHUB_API_TOKEN")"
+  targetBranch=$(echo "$githubResponse" | tr '\r\n' ' ' | ./jq '.base.ref' | tr -d '"')


Is the tr really necessary here to make jq happy? Shouldn't it be able to parse multiline-JSON files without you needing to remove all CRLFs? 🤔

JQ was failing to parse the JSON in some cases and I just used this workaround from stackoverflow that fixed the issue. Not sure why it fixes it though

AliSoftware · 2021-12-08T13:21:56Z

tools/dependency-tree-diff/dependency-tree-diff.sh

+  githubUrl="https://api.github.com/repos/$CIRCLE_PROJECT_USERNAME/$CIRCLE_PROJECT_REPONAME/pulls/$prNumber"
+  githubResponse="$(curl "$githubUrl" -H "Authorization: token $GITHUB_API_TOKEN")"
+  targetBranch=$(echo "$githubResponse" | tr '\r\n' ' ' | ./jq '.base.ref' | tr -d '"')


Note that this whole dance using the GitHub API to retrieve the PR's target branch won't be needed anymore once we migrate to Buildkite, since Buildkite seems to provide the value already via BUILDKITE_PULL_REQUEST_BASE_BRANCH (while the PR's head branch would still be exposed via BUILDKITE_BRANCH similar to the CIRCLE_BRANCH for CircleCI)

One more reason why waiting for the Buildkite migration could help simplify this a lot 🤔

Yeah, I noticed that other CIs usually do have this build-in. Not Circle CI though.

I think the decision on waiting for the migration on Buildkite or using it already now highly depends on time. I think if we talk about weeks then it's ok to wait. If it's months then maybe we could start using this now and then do the migration. It should not be that complicated

Agreed. I think the hope 🤞 is to have the CI migration finally done in client repos by the end of year (overflowing on beginning of January 2022 maybe given holiday break?), in any case very soon.

But I'll let @jkmassel chime in here as here's the one tackling this and knowing the timeline estimate and the progress status on this

Co-authored-by: Olivier Halligon <olivier.halligon@automattic.com>

# Conflicts: # WooCommerce/build.gradle

…' into hack/dependency-tree-diff-integr

kidinov · 2021-12-09T09:55:21Z

@AliSoftware Thank you for your review!

I've just created a dummy PR so the change can be tested in a more comfortable way. //cc @malinajirka

wzieba · 2021-12-27T14:36:46Z

@AliSoftware @kidinov hi folks, thank you for working and reviewing this, I believe it's a great feature!

I understand there are concerns about CircleCI -> BuildKite transition but if this PR in current form is not breaking anything, I'd like to suggest if we could merge it? I believe it adds value as it is and if it'll break something - we can quickly disable it and work on it later.

My one suggestion to PR itself would be, if it's easy to do, to run this only if PR contains changes in *.gradle file. I can't see possibility for dependencies to be changed by editing any other type of file.

WDYT?

kidinov · 2021-12-28T07:30:16Z

I understand there are concerns about CircleCI -> BuildKite transition but if this PR in current form is not breaking anything, I'd like to suggest if we could merge it? I believe it adds value as it is and if it'll break something - we can quickly disable it and work on it later.

I agree, but probably it's not my call

My one suggestion to PR itself would be, if it's easy to do, to run this only if PR contains changes in *.gradle file. I can't see possibility for dependencies to be changed by editing any other type of file.

Good idea! I can take a look into this improvement later - either as a part of this PR another one

jkmassel

This looks good to me, and looks nicely portable, so I have no concerns about migration.

I'd love a quick 👍 from @oguzkocer on the gradle task and on the use of a bare .jar file, but that's the only outstanding item.

jkmassel · 2022-01-11T20:21:55Z

tools/dependency-tree-diff/dependency-tree-diff.sh

+CONFIGURATION="vanillaReleaseRuntimeClasspath"
+
+if [ -n "$CIRCLE_PULL_REQUEST" ]; then
+  curl -L "https://github.com/stedolan/jq/releases/download/jq-1.5/jq-linux64" -o jq


In the future, we should be able to assume the host docker image has jq installed instead of installing it using curl.

Nothing to change for this PR, but it'd be a nice simplification / security improvement.

oguzkocer

@jkmassel @kidinov I left a minor suggestion about eager Gradle task generation. This is not a blocker for this PR, but would be good to include it as otherwise I'd need to create a follow up PR for it.

As for the standalone .jar usage - it's outside of the Gradle toolchain, so I have our typical concerns about using including a binary in our repo. Since it's in the CI process, maybe we can consider downloading it from the source instead. Nevertheless, I don't think that should block this PR, we can open a follow-up PR to that if we can agree on an alternative.

Please note that I only reviewed the Gradle changes of this PR. The rest is already reviewed, so I don't think it's necessary for me to review as well.

WooCommerce/build.gradle

Co-authored-by: Oguz Kocer <oguzkocer@users.noreply.github.com>

…' into hack/dependency-tree-diff-integr

kidinov · 2022-01-13T13:05:41Z

Thanks, everyone! I created the issue with improvements - #5639 Merging it for now!

wzieba · 2022-01-14T10:43:50Z

@kidinov thank you for working on that again! After this PR has been merged, I've rebased Dependabot PRs to check the result and unfortunately the diff hasn't been printed in PR as a comment. Do you have an idea why it could happen?

#5594 and #5252

wzieba · 2022-01-14T11:07:09Z

But it did work on #5643 !

kidinov · 2022-01-14T12:07:51Z

@wzieba
My guess is that both #5594 and #5252 don't bring any runtime dependency changes. The first one is a gradle plugin and the second is androidTest dependencies. #5643 does bring those, that's why probably it worked.

I use vanillaReleaseRuntimeClasspath configuration to determine the list of dependencies. Do you think it might be useful to check when test dependencies have been changed?

wzieba · 2022-01-14T12:15:49Z

You're right, good catch! I had a bad luck (or rather good luck? 😄 ) in choosing PRs.

IMO we shouldn't really bother with test as if our CI passes tests, we should be all good. I don't see any threats here.

But I'm wondering about build classpath - I think it'd be nice for cases like #5521 where we learned that updating safeargs Gradle plugin caused unintentional bump of Kotlin stdlib.

Edit: but if it'll bump Kotlin stdlib, then it'll become runtime dependency so vanillaReleaseRuntimeClasspath will show diff right? I'm checking this in #5521

Edit2: Yeah it showed diff 😄 . So no concerns on my end, thanks again :).

New CI step to check if dependencies were changed

eeeeab0

kidinov requested a review from a team as a code owner December 8, 2021 12:20

kidinov added the type: technical debt Represents or solves tech debt of the project. label Dec 8, 2021

kidinov added this to the 8.3 milestone Dec 8, 2021

kidinov requested a review from malinajirka December 8, 2021 12:23

AliSoftware reviewed Dec 8, 2021

View reviewed changes

kidinov added category: tooling Anything that involves building & maintaining the project, including scripts, `Fastfile`, etc. and removed type: technical debt Represents or solves tech debt of the project. labels Dec 8, 2021

kidinov and others added 4 commits December 9, 2021 12:25

Update tools/dependency-tree-diff/dependency-tree-diff.sh

3154f24

Co-authored-by: Olivier Halligon <olivier.halligon@automattic.com>

Merge branch 'develop' into hack/dependency-tree-diff-integr

b088f41

# Conflicts: # WooCommerce/build.gradle

Merge remote-tracking branch 'origin/hack/dependency-tree-diff-integr…

02036cc

…' into hack/dependency-tree-diff-integr

Improved the script

3f0cc38

kidinov mentioned this pull request Dec 9, 2021

Dummy PR to test dependency diff #5430

Closed

Added a link on dependency-tree-diff

b941d24

AliSoftware requested a review from jkmassel December 20, 2021 13:27

kidinov modified the milestones: 8.3, 8.4 Jan 6, 2022

jkmassel approved these changes Jan 11, 2022

View reviewed changes

oguzkocer reviewed Jan 13, 2022

View reviewed changes

WooCommerce/build.gradle Outdated Show resolved Hide resolved

WooCommerce/build.gradle Outdated Show resolved Hide resolved

kidinov and others added 4 commits January 13, 2022 15:37

Update WooCommerce/build.gradle with a new syntax to register tasks

5b65054

Co-authored-by: Oguz Kocer <oguzkocer@users.noreply.github.com>

Update WooCommerce/build.gradle with a new syntax to register tasks

4f0e037

Co-authored-by: Oguz Kocer <oguzkocer@users.noreply.github.com>

Merge branch 'trunk' into hack/dependency-tree-diff-integr

367d883

Merge remote-tracking branch 'origin/hack/dependency-tree-diff-integr…

11254d4

…' into hack/dependency-tree-diff-integr

kidinov enabled auto-merge January 13, 2022 13:05

kidinov merged commit cd846ec into trunk Jan 13, 2022

kidinov deleted the hack/dependency-tree-diff-integr branch January 13, 2022 13:11

wzieba mentioned this pull request Jan 14, 2022

Bump screengrab from 2.0.0 to 2.1.1 #5252

Merged

oguzkocer mentioned this pull request Jan 26, 2022

Issue/5639 Dependency changes CI step improvements #5675

Merged

1 task

ParaskP7 mentioned this pull request Mar 11, 2022

[HACK WEEK] Add Dependabot & Dependency Tree Diff Configuration wordpress-mobile/WordPress-Android#16092

Merged

3 tasks

Conversation

kidinov commented Dec 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing instructions

Uh oh!

peril-woocommerce bot commented Dec 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AliSoftware left a comment

Choose a reason for hiding this comment

Uh oh!

AliSoftware Dec 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AliSoftware Dec 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kidinov Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kidinov commented Dec 9, 2021

Uh oh!

wzieba commented Dec 27, 2021

Uh oh!

kidinov commented Dec 28, 2021

Uh oh!

jkmassel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oguzkocer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kidinov commented Jan 13, 2022

Uh oh!

wzieba commented Jan 14, 2022

Uh oh!

wzieba commented Jan 14, 2022

Uh oh!

kidinov commented Jan 14, 2022

Uh oh!

wzieba commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kidinov commented Dec 8, 2021 •

edited

Loading

peril-woocommerce bot commented Dec 8, 2021 •

edited

Loading

AliSoftware Dec 8, 2021 •

edited

Loading

AliSoftware Dec 8, 2021 •

edited

Loading

kidinov Dec 9, 2021 •

edited

Loading

wzieba commented Jan 14, 2022 •

edited

Loading