Skip to content

Conversation

henrybear327
Copy link
Contributor

@henrybear327 henrybear327 commented Sep 19, 2024

Based on the experience of performing dependency bumps, some minor improvements are made to the script to make it conform to our current dependency bump procedure, listed as follows:

  • print out the dependency's version before and after the bump
  • check if the dependency is fully indirect
  • change the behavior of bumping dependency (doesn't ignore bumping indirect dependency in the go mod files anymore)
  • check if all dependencies across all go mod files have the same pinned version respectively after bumping a dependency

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

@henrybear327
Copy link
Contributor Author

/cc @ivanvc
/cc @ahrtr
I am not good with bash scripts :(

This is the script that I have been using to bump the dependencies in the past months. Hopefully, it will be helpful for future volunteers before the dependabot is fixed!

@ivanvc
Copy link
Member

ivanvc commented Sep 19, 2024

@henrybear327, there are some shellcheck warnings in the script. Would you want to draft the PR? And would you like me to continue on top of it? Or do you want to address the issues?

@henrybear327
Copy link
Contributor Author

@henrybear327, there are some shellcheck warnings in the script. Would you want to draft the PR? And would you like me to continue on top of it? Or do you want to address the issues?

@ivanvc let's draft the PR and you can probably take over it if you have time to improve it!

Hopefully it's a helpful start otherwise you can trash the PR and start from scratch!

Thanks!

@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2024

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.95%. Comparing base (aeb47ee) to head (519a1ff).

⚠️ Current head 519a1ff differs from pull request most recent head f159f38

Please upload reports for the commit f159f38 to get more accurate results.

Additional details and impacted files

see 145 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #18609      +/-   ##
==========================================
+ Coverage   68.92%   68.95%   +0.02%     
==========================================
  Files         417      420       +3     
  Lines       34648    35762    +1114     
==========================================
+ Hits        23882    24659     +777     
- Misses       9350     9679     +329     
- Partials     1416     1424       +8     

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aeb47ee...f159f38. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@henrybear327
Copy link
Contributor Author

@henrybear327, there are some shellcheck warnings in the script. Would you want to draft the PR? And would you like me to continue on top of it? Or do you want to address the issues?

@ivanvc I have fixed the shellcheck errors

Maybe you can see if this is a good enough quality script to consider now!
Thank you!

Comment on lines 35 to 37
if grep --exclude-dir=.git --include=\*.mod -Ri -q "^.*${mod} v.*// indirect$"; then
echo "Fully indirect, we will terminate the script"
exit 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes we still need bump pure indirect dependency, i.e. due to CVE. A couple of approaches:

  • raise a question something "XXX is a pure indirect dependency, Are you sure you want to proceed? (y/n):"
  • Or we can just print a warning and automatically continue to execute the script. As mentioned in previous comment, it's up to maintainers/contributors whether to bump a pure indirect dependency. If not, then they shouldn't run this script at all.

@henrybear327 henrybear327 force-pushed the ci/improve_update_dep branch 2 times, most recently from 967bc31 to f892b05 Compare September 25, 2024 07:33
@henrybear327 henrybear327 marked this pull request as draft September 25, 2024 07:55
Copy link
Member

@ivanvc ivanvc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pull request, Henry. I haven't had a chance to check it before. I left some comments ✌️

function maybe_update_module {
function print_current_dep_version {
echo "${mod} version in all go mod files"
grep --exclude-dir=.git --include=\*.mod -Ri "^.*${mod} v.*$" | grep -v sum
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By passing --include to the first grep, I don't think you need to pipe the second grep. It won't match go.sum files.

I think your regular expression can be simplified to "${mod} v". I don't see the value of ^.* and .*$, which matches anything before and after. I'd suggest simplifying.

Comment on lines 31 to 50
# check if all lines end with "// indirect"
# if grep found nothing, the error code will be non-zero
ALL=$(grep --exclude-dir=.git --include=\*.mod -Ri "^.*${mod} v.*$" | grep -v sum | wc -l)
ONLY_INDIRECT=$(grep --exclude-dir=.git --include=\*.mod -Ri "^.*${mod} v.*// indirect$" | grep -v sum | wc -l)
if [[ "$ALL" == "$ONLY_INDIRECT" ]]; then
echo "Fully indirect, we will terminate the script"
exit 1
else
echo "Not fully indirect, we will perform dependency bump"
fi
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another approach would be to use go list for this, i.e., something like:

local result
result=$(find . -name go.mod | xargs -I{} /bin/sh -c 'cd $(dirname {}); go list -f "{{if eq .Path \"'"${mod}"'\"}}{{.Indirect}}{{end}}" -m all' | sort | uniq)
if [ "$result" = "true" ] ; then
   read -p "Module ${mod} is an indirect dependency. Are you sure you want to update it? [y/N] " -r confirm
   [[ "${confirm,,}" == "y" ]] || exit
else
  echo "Not fully..."
fi

@henrybear327 henrybear327 force-pushed the ci/improve_update_dep branch 3 times, most recently from 1e47ce2 to 97d9610 Compare October 8, 2024 17:36
@henrybear327 henrybear327 force-pushed the ci/improve_update_dep branch 2 times, most recently from 401bceb to 612ca28 Compare October 27, 2024 03:33
@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: henrybear327
Once this PR has been reviewed and has the lgtm label, please assign ivanvc for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@henrybear327 henrybear327 force-pushed the ci/improve_update_dep branch 2 times, most recently from 4b83871 to 786139e Compare January 28, 2025 13:51
@henrybear327 henrybear327 force-pushed the ci/improve_update_dep branch 2 times, most recently from 255732d to 969cba1 Compare February 10, 2025 19:52
@henrybear327 henrybear327 force-pushed the ci/improve_update_dep branch 2 times, most recently from 721a42e to 2d701d5 Compare March 27, 2025 15:22
@henrybear327 henrybear327 force-pushed the ci/improve_update_dep branch from 2d701d5 to 2008fbc Compare April 8, 2025 06:48
@henrybear327 henrybear327 force-pushed the ci/improve_update_dep branch from 2008fbc to 190eb93 Compare April 16, 2025 18:14
@k8s-ci-robot
Copy link

@henrybear327: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-etcd-verify f892b05 link true /test pull-etcd-verify
ci-etcd-robustness-release36-amd64 190eb93 link true /test ci-etcd-robustness-release36-amd64

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@henrybear327 henrybear327 force-pushed the ci/improve_update_dep branch from 190eb93 to 67437b0 Compare May 28, 2025 05:26
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added stale and removed stale labels Jul 28, 2025
@henrybear327 henrybear327 force-pushed the ci/improve_update_dep branch from 67437b0 to c370497 Compare August 2, 2025 08:09
@henrybear327 henrybear327 force-pushed the ci/improve_update_dep branch from c370497 to 488ca41 Compare August 13, 2025 07:39
@henrybear327 henrybear327 force-pushed the ci/improve_update_dep branch from 488ca41 to 0878b1f Compare August 27, 2025 19:26
henrybear327 and others added 2 commits September 9, 2025 07:30
Based on the experience of performing dependency bumps, some minor
improvements are made to the script to make it conform to our current
dependency bump procedure, listed as follows:
- print out the dependency's version before and after the bump
- check if the dependency is fully indirect
- change the behavior of bumping dependency (doesn't ignore bumping
indirect dependency in the go mod files anymore)
- check if all dependencies across all go mod files have the same pinned
version respectively after bumping a dependency

Signed-off-by: Chun-Hung Tseng <[email protected]>
Signed-off-by: Chun-Hung Tseng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants