Update update_dep.sh #18609

henrybear327 · 2024-09-19T17:01:42Z

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.

k8s-ci-robot · 2024-09-19T17:01:47Z

[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 spzala for approval. For more information see the Kubernetes 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:

OWNERS

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

henrybear327 · 2024-09-19T17:07:44Z

/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 · 2024-09-19T17:14:40Z

@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 · 2024-09-19T17:17:02Z

@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 · 2024-09-19T17:22:16Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.76%. Comparing base (1a08fb2) to head (7c6a4ec).

❗ Current head 7c6a4ec differs from pull request most recent head d26e944

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

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

see 26 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #18609      +/-   ##
==========================================
+ Coverage   68.74%   68.76%   +0.02%     
==========================================
  Files         420      420              
  Lines       35535    35535              
==========================================
+ Hits        24427    24436       +9     
+ Misses       9677     9667      -10     
- Partials     1431     1432       +1

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 1a08fb2...d26e944. Read the comment docs.

henrybear327 · 2024-09-22T15:55:29Z

@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!

scripts/update_dep.sh

ahrtr · 2024-09-23T06:48:40Z

scripts/update_dep.sh

+  if grep --exclude-dir=.git --include=\*.mod -Ri -q "^.*${mod} v.*// indirect$"; then 
+      echo "Fully indirect, we will terminate the script"
+      exit 1


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.

k8s-ci-robot · 2024-09-25T07:39:49Z

@henrybear327: The following test 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`

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.

ivanvc

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

ivanvc · 2024-09-25T18:24:18Z

scripts/update_dep.sh

-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


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.

ivanvc · 2024-09-25T18:27:01Z

scripts/update_dep.sh

  run go mod tidy

-  deps=$(go list -f '{{if not .Indirect}}{{if .Version}}{{.Path}},{{.Version}}{{end}}{{end}}' -m all)
+  deps=$(go list -f '{{if .Version}}{{.Path}},{{.Version}}{{end}}' -m all)
  if [[ "$deps" == *"${mod}"* ]]; then
    if [ -z "${ver}" ]; then


With the changes from the top of the file

if [ "$#" -ne 2 ]; then echo "Illegal number of parameters" exit 1 fi

We will never reach this conditional, as ${ver} will never be empty.

ivanvc · 2024-09-25T18:45:53Z

scripts/update_dep.sh

+  # 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


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

ivanvc · 2024-09-25T19:01:08Z

scripts/update_dep.sh

+# check all dependencies across all go mod files have the same pinned version respectively
+PASSES="dep" ./scripts/test.sh

-go mod tidy
-run_for_modules maybe_update_module
+./scripts/fix.sh


Shouldn't we invert these steps? Shouldn't it be first fix, then the dep tests?

ivanvc · 2024-09-25T19:05:53Z

scripts/update_dep.sh

+if [ "$#" -ne 2 ]; then
+    echo "Illegal number of parameters"
+    exit 1
+fi


This also outdates the documentation at the top of the file 😅

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]>

k8s-ci-robot added the size/M label Sep 19, 2024

henrybear327 force-pushed the ci/improve_update_dep branch from 3976754 to c0ba3eb Compare September 19, 2024 17:02

henrybear327 mentioned this pull request Sep 19, 2024

[2024-09-19] Bump dependency updates identified by dependabot #18610

Merged

k8s-ci-robot requested review from ahrtr and ivanvc September 19, 2024 17:07

henrybear327 marked this pull request as draft September 20, 2024 08:17

k8s-ci-robot added the do-not-merge/work-in-progress label Sep 20, 2024

henrybear327 force-pushed the ci/improve_update_dep branch from c0ba3eb to 4691dfc Compare September 22, 2024 15:53

henrybear327 marked this pull request as ready for review September 22, 2024 15:54

k8s-ci-robot removed the do-not-merge/work-in-progress label Sep 22, 2024

ahrtr reviewed Sep 22, 2024

View reviewed changes

scripts/update_dep.sh Outdated Show resolved Hide resolved

henrybear327 force-pushed the ci/improve_update_dep branch from 4691dfc to f94117c Compare September 22, 2024 19:27

henrybear327 requested a review from ahrtr September 22, 2024 19:28

ahrtr reviewed Sep 23, 2024

View reviewed changes

henrybear327 force-pushed the ci/improve_update_dep branch 2 times, most recently from 967bc31 to f892b05 Compare September 25, 2024 07:33

henrybear327 marked this pull request as draft September 25, 2024 07:55

k8s-ci-robot added the do-not-merge/work-in-progress label Sep 25, 2024

ivanvc reviewed Sep 25, 2024

View reviewed changes

henrybear327 force-pushed the ci/improve_update_dep branch from f892b05 to d26e944 Compare September 25, 2024 20:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update update_dep.sh #18609

Update update_dep.sh #18609

henrybear327 commented Sep 19, 2024 •

edited

Loading

k8s-ci-robot commented Sep 19, 2024

henrybear327 commented Sep 19, 2024

ivanvc commented Sep 19, 2024

henrybear327 commented Sep 19, 2024

codecov-commenter commented Sep 19, 2024 •

edited

Loading

henrybear327 commented Sep 22, 2024

ahrtr Sep 23, 2024

k8s-ci-robot commented Sep 25, 2024

ivanvc left a comment

ivanvc Sep 25, 2024

ivanvc Sep 25, 2024

ivanvc Sep 25, 2024

ivanvc Sep 25, 2024

ivanvc Sep 25, 2024

Update update_dep.sh #18609

Are you sure you want to change the base?

Update update_dep.sh #18609

Conversation

henrybear327 commented Sep 19, 2024 • edited Loading

k8s-ci-robot commented Sep 19, 2024

henrybear327 commented Sep 19, 2024

ivanvc commented Sep 19, 2024

henrybear327 commented Sep 19, 2024

codecov-commenter commented Sep 19, 2024 • edited Loading

Codecov Report

henrybear327 commented Sep 22, 2024

ahrtr Sep 23, 2024

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 25, 2024

ivanvc left a comment

Choose a reason for hiding this comment

ivanvc Sep 25, 2024

Choose a reason for hiding this comment

ivanvc Sep 25, 2024

Choose a reason for hiding this comment

ivanvc Sep 25, 2024

Choose a reason for hiding this comment

ivanvc Sep 25, 2024

Choose a reason for hiding this comment

ivanvc Sep 25, 2024

Choose a reason for hiding this comment

henrybear327 commented Sep 19, 2024 •

edited

Loading

codecov-commenter commented Sep 19, 2024 •

edited

Loading