Comment with dependency diff #120
Conversation
wzieba
force-pushed
the
comment_with_dependency_diff
branch
from
91b617f to
b1d21df
Compare
Instead of a Gradle plugin, which would have to be managed on project source code
It's more readable this way
…e adding content to comment
Interpret the content only as markdown
For debugging
…tallation). We don't want to hold the script execution for this reason.
To maybe fix `line 129: /var/lib/buildkite-agent/.rbenv/shims/bundle: Argument list too long`
like in upload_sarif_to_github
Previously script would crash if project diff and build env diff had both e.g. 50k characters.
Instead, get this from BUILDKITE env variable. I took this idea from comment_on_pr script.
bin/comment_on_pr
Outdated
| # Use jq with the temporary file for payload | ||
| #jq -c --null-input --arg body "$(cat "$comment_body_file")" '{body: $body}' > "$json_payload_file" | ||
|
|
There was a problem hiding this comment.
🧹 I guess this is leftover from debugging and can be removed?
bin/comment_on_pr
Outdated
| json=$(jq -c --null-input \ | ||
| --rawfile body "$comment_body_file" \ | ||
| '{ | ||
| body: $body | ||
| } | ||
| ' | ||
| ) | ||
|
|
||
| json_payload_file=$(mktemp) | ||
| echo "$json" > "$json_payload_file" |
There was a problem hiding this comment.
💄 Alternative: avoid intermediate json variable altogether:
| json=$(jq -c --null-input \ | |
| --rawfile body "$comment_body_file" \ | |
| '{ | |
| body: $body | |
| } | |
| ' | |
| ) | |
| json_payload_file=$(mktemp) | |
| echo "$json" > "$json_payload_file" | |
| json_payload_file=$(mktemp) | |
| jq -c --null-input \ | |
| --rawfile body "$comment_body_file" \ | |
| '{ body: $body }' \ | |
| > "$json_payload_file" |
| @@ -0,0 +1,153 @@ | |||
| #!/bin/bash | |||
|
|
|||
| set -euo pipefail | |||
There was a problem hiding this comment.
📚 The more we add utility scripts like those to this plugin, the more I realize we should have made a better job as providing help / documentation at what each of those do. Especially so that people not as familiar with our CI (e.g. devs that are interested in using some of those helper scripts in their pipelines themselves but have no idea what each utility does) can better understand the purpose of each.
I wish we took that habit sooner and applied it to all of our other bin/* utilities in this repo, making them all start with a code-comment explaining their role / context in which those helpers would be used and could help, documenting what the script does and its expected parameters and inputs (e.g. "this script should only run after ./gradlew … has been called and generated the build artifacts", etc).
We should probably make a pass at all of them at some point.
In the meantime, wdyt about us already starting moving towards this good habit and add some intro doc comments in any new helpers we introduce, starting from this one?
bin/comment_with_dependency_diff
Outdated
| DIFF_BUILD_ENV_FILE="$DIFF_DEPENDENCIES_FOLDER/diff_build_env.txt" | ||
| DEPENDENCY_TREE_VERSION="1.2.1" | ||
|
|
||
| PR_NUMBER=$BUILDKITE_PULL_REQUEST |
There was a problem hiding this comment.
💡 Even if one is only expected to call this helper on a Buildkite step for which we set if: build.pull_request.id != null, could be worth adding some protection about it anyway—typically at the start of the script, after your tests for -z "$MODULE" / -z "$CONFIGURATION". Like:
if [ "${BUILDKITE_PULL_REQUEST:-false}" == "false" ]; then
echo "This script should only be called on pull requests. Be sure to set `if: build.pull_request.id != null` on the step invoking it in your buildkite pipeline"
exit 2
fi
bin/comment_with_dependency_diff
Outdated
|
|
||
| echo "--> Starting the check" | ||
| echo "--> Fetching the base branch from $PR_URL" | ||
| githubResponse="$(curl "$PR_URL" -H "Authorization: token $GITHUB_TOKEN")" |
There was a problem hiding this comment.
💭 I have not checked if this would be strictly equivalent or not, but what do you think about using our github_api helper from this repo instead of calling curl directly, now that this script lives in the same repo/plugin? 🤔
bin/comment_with_dependency_diff
Outdated
| githubResponse="$(curl "$PR_URL" -H "Authorization: token $GITHUB_TOKEN")" | ||
| baseBranch=$(echo "$githubResponse" | tr '\r\n' ' ' | jq '.base.ref' | tr -d '"') | ||
| echo "--> Base branch is $baseBranch" |
There was a problem hiding this comment.
🤔 Actually, is the purpose of this whole API call only to extract the name of the base (aka target) branch of the PR?
If so, why not just use BUILDKITE_PULL_REQUEST_BASE_BRANCH directly?
(I think I slight remember mentioning this a while ago when I reviewed the first iterations of this script back when it was added to the repositories themselves, and iirc the argument was mostly due to legacy with this script previously being also used in other CI, maybe while we were still in the middle of transitioning to Buildkite…? So maybe that's where it comes from?)
There was a problem hiding this comment.
ah yes, thank you. It simplified things a lot!
bin/comment_with_dependency_diff
Outdated
| echo "--> Downloading dependency-tree-diff.jar" | ||
| curl -v -L "https://github.com/JakeWharton/dependency-tree-diff/releases/download/$DEPENDENCY_TREE_VERSION/dependency-tree-diff.jar" -o dependency-tree-diff.jar | ||
| sha=$(sha1sum dependency-tree-diff.jar | cut -d' ' -f1) | ||
| if [[ $sha != "39c50f22cd4211a3e52afec43120b7fdf90f9890" ]]; then |
There was a problem hiding this comment.
💡 Nitpick: could be worth extracting this SHA in a DEPENDENCY_TREE_CHECKSUM constant to put next to line 23, so that it'd be easier to update to a new version in the future by changing the version and checksum together?
bin/comment_with_dependency_diff
Outdated
| current_file_size=$(wc -c < "$COMMENT_FILE") | ||
|
|
||
| if (( current_file_size + ${#content} > github_pr_comment_max_size )); then | ||
| echo "Content exceeds $github_pr_comment_max_size characters. Navigate to Buildkite build artifacts to see details." >> $COMMENT_FILE |
There was a problem hiding this comment.
💡 I think we could even add a link to the job for quicker access (users would still need to click on the "Artifacts" tab but at least they'd already be on the right build and job 🙂 )
| echo "Content exceeds $github_pr_comment_max_size characters. Navigate to Buildkite build artifacts to see details." >> $COMMENT_FILE | |
| echo "Content exceeds $github_pr_comment_max_size characters. [Navigate to Buildkite build artifacts](${BUILDKITE_BUILD_URL}#${BUILDKITE_JOB_ID}) to see details." >> $COMMENT_FILE |
There was a problem hiding this comment.
Btw:
- There's a risk that
current_file_sizewould be so close togithub_pr_comment_max_sizealready that even just adding this "Content exceed …" line to it will make it go over the limit…- So the
github_pr_comment_max_sizelimit value should really be 65536… minus the length of this line telling that the content exceeds the max size 😅
- So the
- Wouldn't the logic of
append_contenthere add theecho "Content exceeds …"string multiple times if you callappend_contentmultiple times and it's not just the last one but one before that that makes the length be exceeded?- Though I guess in this case it's not a big deal because you only call
append_contenta maximum number of 2 times, so this scenario can only happen if the content fromDIFF_DEPENDENCIES_FILEis already too long, and in which case you'll print theContent exceeds …2 times (one for when you call it forDIFF_DEPENDENCIES_FILE, then other when you call it forDIFF_BUILD_ENV_FILE)… so probably not that dramatic and not worth fixing I guess. - But just wanted to mention in case you wanted to look into it, or if you planned in future PRs to potentially add more info and call
append_contentmore times in future iterations of this helper, just to keep that in mind.
- Though I guess in this case it's not a big deal because you only call
…ch from GH API It simplifies things a lot!
To match all other vars
for easier updates in the future
|
Thank you @AliSoftware for the review! You raised good points. I think I've adjusted the implementation. Here are also updated tests:
Both seem to still work fine. |
|
Correction, title is missing 
|
|
We're back on track! |
|
I just realized that there might be more to it to compute the exact comment body length, because:
Maybe the easiest solution is to simply reduce the value of |
Description
This PR introduces
comment_with_dependency_diffscript that is aiming to help Android product teams to better understand what changes to the dependency tree a PR might introduce.We use similar scripts in WooCommerce and Day One projects. I want to extend support to other projects and share the implementation between them for easier maintenance.
Additionally, this PR introduces some improvements to the script:
To see the result, please see:
CHANGELOG.mdif necessary.