Add tools/mod to module_dirs #18590

henrybear327 · 2024-09-14T20:49:42Z

As tools/mod also contains the go.mod file, we should look into adding it to the module_dirs variable, so that when executing ./scripts/fix.sh, the proper checks and fixes can be applied.

To address the issue of broken unit tests and code coverage due to tools/mod directory's lack of Go code, we've introduced a new doc.go file. This file acts as a placeholder, enabling tools like golangci-lint and go test to function correctly.

Discovered when working on #18575, where running ./scripts/fix.sh missed fixing the go mod file in tools/mod, causing the CI pipeline to fail.

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

henrybear327 · 2024-09-14T20:53:24Z

Need to rebase on main after #18575 is merged

Currently CI will error on

cd tools/mod && golangci-lint run --config /Users/henrybear327/go/src/etcd/tools/.golangci.yaml ./...
ERRO Running error: context loading failed: no go files to analyze: running `go mod tidy` may solve the problem

codecov-commenter · 2024-09-14T21:07:36Z

⚠️ 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 (2ed418c) to head (b5327f4).

❗ Current head b5327f4 differs from pull request most recent head fc901bd

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

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

Additional details and impacted files

see 27 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #18590      +/-   ##
==========================================
- Coverage   68.83%   68.76%   -0.08%     
==========================================
  Files         420      420              
  Lines       35474    35519      +45     
==========================================
+ Hits        24418    24424       +6     
- Misses       9636     9669      +33     
- Partials     1420     1426       +6

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 2ed418c...fc901bd. Read the comment docs.

henrybear327 · 2024-09-16T08:55:44Z

/cc @ivanvc

henrybear327 · 2024-09-16T09:32:54Z

/retest

ivanvc

Thanks for catching and addressing this, Henry! I left a suggestion, but if we don't want to block the PR, I'm okay with a follow-up to address them.

tools/mod/mod.go

henrybear327 · 2024-09-16T17:42:13Z

Let’s make sure this PR is perfect before merging! Doing a follow-up will make the codebase commit history ugly :(

ivanvc

LGTM. Thanks, Henry.

jmhbnz

LGTM

cc @serathius, @ahrtr in case there is some historical reason why tools/mod was intentionally excluded that I am not aware of.

ivanvc · 2024-09-17T23:53:24Z

Trying to shed more context. We're currently excluding it from modules (ref: #17770), as we don't want to include its dependencies in the bill of materials. But I think it should be fine (and sound) to add them to module_dirs so they get checked by Makefile's verify target.

Edit: But please chime in, Marek and Benjamin, if this may be problematic.

ahrtr · 2024-09-18T09:03:58Z

tools/mod/doc.go

+// This would break scripts for unit testing and coverage calculation. However,
+// to ensure tools like golangci-lint and go test run normally and perform go mod checks,
+// we've added this empty file.


Can you explain this? Why it would break scripts for unit testing and coverage calculation? also why it has some impact on golangci-lint and go test?

Yes definitely @ahrtr!

Please see https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/etcd-io_etcd/18590/pull-etcd-unit-test-386/1835058579317460992

As you can see, the main issue is

% (cd tools/mod && 'env' 'ETCD_VERIFY=all' 'go' 'test' '-v' '-json' '-p=4' '-short' '-timeout=3m' '--race=false' '--cpu=1' './...') stderr: go: warning: "./..." matched no packages stderr: no packages to test

As we don't have any source file in the directory, ./... will cause the tools to report errors, thus, causing the CI to fail

What do you think? :)

Thanks for the clarification. It seems the reason is the module name go.etcd.io/etcd/tools/v3 is different from the directory name mod?

Please add this into the comment to avoid any confusion for now.

Also why not to make them consistent to avoid such unnecessary confusing comment? It's OK to resolve it separately.

ahrtr · 2024-09-18T09:05:53Z

as we don't want to include its dependencies in the bill of materials.

I might have lost some context. You added the tools/mod into the list, but why BOM wasn't changed?

henrybear327 · 2024-09-18T09:21:13Z

as we don't want to include its dependencies in the bill of materials.

I might have lost some context. You added the tools/mod into the list, but why BOM wasn't changed?

This I actually don't know. @ivanvc do you have some idea? :)

As `tools/mod` also contains the `go.mod` file. We should add it to the `module_dirs` variable, so that when executing `./scripts/fix.sh`, the proper checks and fixes can be applied. To address the issue of broken unit tests and code coverage due to the directory's lack of Go code, we've introduced a new doc.go file. This file acts as a placeholder, enabling tools like golangci-lint and go test to function correctly. --- Discovered when working on etcd-io#18575 The directories are checked against the following: - Command: `find . -type f -name go.mod -exec dirname {} \;` - Output: ``` ./etcdutl . ./tools/testgrid-analysis ./tools/rw-heatmaps ./tools/mod ./etcdctl ./tests ./server ./api ./client/internal/v2 ./client/v3 ./client/pkg ./pkg ``` Signed-off-by: Chun-Hung Tseng <henrybear327@gmail.com>

ivanvc · 2024-09-18T16:59:15Z

This I actually don't know. @ivanvc do you have some idea? :)

It is definitely not straightforward; I hope that with the Go workspace, we can clean up/simplify some of these workflows.

I'll break down the execution stack step by step to try to make it clear for everyone.

Calling make fix includes updating the bill of materials:

etcd/Makefile

Lines 72 to 74 in 0430960

    
           .PHONY: fix 
        
           fix: fix-bom fix-lint fix-yamllint sync-toolchain-directive 
        
           	./scripts/fix.sh

We'll focus on the fix-bom target. It executes scripts/updatebom.sh:

etcd/Makefile

Lines 84 to 86 in 0430960

.PHONY: fix-bom

fix-bom:

./scripts/updatebom.sh

Executing scripts/updatebom.sh runs the bom_fix function defined in that same file:

etcd/scripts/updatebom.sh

Lines 39 to 42 in c4499ad

    
           # only build when called directly, not sourced 
        
           if [[ "$0" =~ updatebom.sh$ ]]; then 
        
             bom_fix 
        
           fi

Note that this script sources scripts/tests_lib.sh:

etcd/scripts/updatebom.sh

Line 5 in c4499ad

source ./scripts/test_lib.sh

The bom_fix function calls the bom_fixlet function defined in that same file:

etcd/scripts/updatebom.sh

Lines 32 to 37 in c4499ad

    
           function bom_fix { 
        
             # We regenerate bom from the tests directory, as it's a module 
        
             # that depends on all other modules, so we can generate comprehensive content. 
        
             # TODO: Migrate to root module, when root module depends on everything (including server & tests). 
        
             run_for_module "." bom_fixlet 
        
           }

bom_fixlet will iterate over modules_exp defined in scripts/test_lib.sh:

etcd/scripts/updatebom.sh

Lines 7 to 19 in c4499ad

    
           function bom_fixlet { 
        
             log_callout "generating bill-of-materials.json" 
        
             cp go.mod go.mod.tmp 
        
             cp go.sum go.sum.tmp 
        
             local modules 
        
             # shellcheck disable=SC2207 
        
             modules=($(modules_exp)) 
        
             if GOFLAGS=-mod=mod run_go_tool "github.com/appscodelabs/license-bill-of-materials" \ 
        
                 --override-file ./bill-of-materials.override.json \ 
        
                 "${modules[@]}" > ./bill-of-materials.json.tmp; then

Finally, modules_exp has the following definition (please make sure to scroll down in the code snippet area to reach modules_exp):

etcd/scripts/test_lib.sh

Lines 181 to 203 in e094139

    
           # modules 
        
           # returns the list of all modules in the project, not including the tools, 
        
           # as they are not considered to be added to the bill for materials. 
        
           function modules() { 
        
             modules=( 
        
               "${ROOT_MODULE}/api/v3" 
        
               "${ROOT_MODULE}/pkg/v3" 
        
               "${ROOT_MODULE}/client/pkg/v3" 
        
               "${ROOT_MODULE}/client/v2" 
        
               "${ROOT_MODULE}/client/v3" 
        
               "${ROOT_MODULE}/server/v3" 
        
               "${ROOT_MODULE}/etcdutl/v3" 
        
               "${ROOT_MODULE}/etcdctl/v3" 
        
               "${ROOT_MODULE}/tests/v3" 
        
               "${ROOT_MODULE}/v3") 
        
             echo "${modules[@]}" 
        
           } 
        
           function modules_exp() { 
        
             for m in $(modules); do 
        
               echo -n "${m}/... " 
        
             done 
        
           }

Therefore, running the BOM update script will not run over any of the tools modules.

ahrtr · 2024-09-18T17:40:31Z

Thanks @ivanvc for the clarification. So we have a dedicated modules (the modules_exp) for BOM. In that case, can we rename modules_exp to something like modules_for_bom?

ivanvc · 2024-09-18T17:45:40Z

Thanks @ivanvc for the clarification. So we have a dedicated modules (the modules_exp) for BOM. In that case, can we rename modules_exp to something like modules_for_bom?

I just verified that modules_exp is just called by the BOM tasks. So, this is a great suggestion. Would you suggest doing it in a follow-up?

Edit: Maybe even a more ambitious task would be to break the scripts files into files with a single goal/target/concern. i.e., create a bom.sh file that contains all the functions, variable definitions, etc., related to updating/verifying the BOM. However, this is a more significant effort, and I would want to consider it after we rework the scripts because of the Go workspace.

ahrtr · 2024-09-18T17:57:21Z

create a bom.sh file that contains all the functions

Seems like a good idea, but let's do it separately.

Two followups,

try to get rid of the confusing comment (and optionally remove the unnecessary doc.go) as mentioned in Add tools/mod to module_dirs #18590 (comment)
rename modules_exp to something like modules_for_bom as mentioned in Add tools/mod to module_dirs #18590 (comment)

When above two followups are done, I am open to do minor refactor as mentioned above #18590 (comment)

ahrtr

LGTM

Thanks

k8s-ci-robot · 2024-09-18T17:57:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, henrybear327, ivanvc, jmhbnz

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ahrtr,jmhbnz]

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

ahrtr · 2024-09-18T17:58:34Z

Could anyone raise a ticket to track #18590 (comment) before we merge this PR? Thanks

ivanvc · 2024-09-19T00:04:14Z

Could anyone raise a ticket to track #18590 (comment) before we merge this PR? Thanks

I opened #18602 to track the rename from modules_exp to modules_for_bom.

@henrybear327, do you want to do the following in this same PR, or should we do it in a follow-up?

try to get rid of the confusing comment #18590 (comment)

Regarding:

and optionally remove the unnecessary doc.go

@ahrtr, I think due to golanci-lint, we need to add the documentation for the package, that's why I suggested naming it doc.go. But I'm also okay with implementing something similar to the root's dummy.go, and add an exception if the linter fails. Basically rename the file from doc.go to dummy.go and remove the module "documentation."

ahrtr · 2024-09-19T09:53:42Z

@ahrtr, I think due to golanci-lint, we need to add the documentation for the package, that's why I suggested naming it doc.go. But I'm also okay with implementing something similar to the root's dummy.go, and add an exception if the linter fails. Basically rename the file from doc.

OK. #18607 (comment)

k8s-ci-robot added the size/XS label Sep 14, 2024

henrybear327 force-pushed the ci/fix branch from e5e43c5 to d98e743 Compare September 14, 2024 20:50

henrybear327 marked this pull request as draft September 14, 2024 20:56

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

henrybear327 force-pushed the ci/fix branch 2 times, most recently from 60ed2ae to 117598a Compare September 16, 2024 08:47

k8s-ci-robot added area/tooling size/S and removed size/XS labels Sep 16, 2024

henrybear327 marked this pull request as ready for review September 16, 2024 08:55

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

k8s-ci-robot requested a review from ivanvc September 16, 2024 08:55

henrybear327 force-pushed the ci/fix branch from 117598a to 8dd4a56 Compare September 16, 2024 09:02

ivanvc reviewed Sep 16, 2024

View reviewed changes

tools/mod/mod.go Outdated Show resolved Hide resolved

tools/mod/mod.go Outdated Show resolved Hide resolved

henrybear327 force-pushed the ci/fix branch from 8dd4a56 to aa7efbd Compare September 17, 2024 20:56

ivanvc approved these changes Sep 17, 2024

View reviewed changes

jmhbnz approved these changes Sep 17, 2024

View reviewed changes

k8s-ci-robot added the approved label Sep 17, 2024

ahrtr reviewed Sep 18, 2024

View reviewed changes

henrybear327 force-pushed the ci/fix branch from aa7efbd to c5e4177 Compare September 18, 2024 09:20

henrybear327 force-pushed the ci/fix branch from c5e4177 to fc901bd Compare September 18, 2024 12:33

ahrtr approved these changes Sep 18, 2024

View reviewed changes

ivanvc mentioned this pull request Sep 18, 2024

Rename scripts' modules_exp to modules_for_bom #18602

Closed

ahrtr merged commit 9fc3b2a into etcd-io:main Sep 19, 2024
43 checks passed

henrybear327 deleted the ci/fix branch September 19, 2024 09:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tools/mod to module_dirs #18590

Add tools/mod to module_dirs #18590

henrybear327 commented Sep 14, 2024 •

edited

Loading

henrybear327 commented Sep 14, 2024 •

edited

Loading

codecov-commenter commented Sep 14, 2024 •

edited

Loading

henrybear327 commented Sep 16, 2024

henrybear327 commented Sep 16, 2024

ivanvc left a comment

henrybear327 commented Sep 16, 2024

ivanvc left a comment

jmhbnz left a comment

ivanvc commented Sep 17, 2024 •

edited

Loading

ahrtr Sep 18, 2024

henrybear327 Sep 18, 2024

ahrtr Sep 18, 2024

ahrtr Sep 18, 2024

ahrtr commented Sep 18, 2024 •

edited

Loading

henrybear327 commented Sep 18, 2024

ivanvc commented Sep 18, 2024 •

edited

Loading

ahrtr commented Sep 18, 2024

ivanvc commented Sep 18, 2024 •

edited

Loading

ahrtr commented Sep 18, 2024

ahrtr left a comment

k8s-ci-robot commented Sep 18, 2024

ahrtr commented Sep 18, 2024

ivanvc commented Sep 19, 2024 •

edited

Loading

ahrtr commented Sep 19, 2024

Add tools/mod to module_dirs #18590

Add tools/mod to module_dirs #18590

Conversation

henrybear327 commented Sep 14, 2024 • edited Loading

henrybear327 commented Sep 14, 2024 • edited Loading

codecov-commenter commented Sep 14, 2024 • edited Loading

Codecov Report

henrybear327 commented Sep 16, 2024

henrybear327 commented Sep 16, 2024

ivanvc left a comment

Choose a reason for hiding this comment

henrybear327 commented Sep 16, 2024

ivanvc left a comment

Choose a reason for hiding this comment

jmhbnz left a comment

Choose a reason for hiding this comment

ivanvc commented Sep 17, 2024 • edited Loading

ahrtr Sep 18, 2024

Choose a reason for hiding this comment

henrybear327 Sep 18, 2024

Choose a reason for hiding this comment

ahrtr Sep 18, 2024

Choose a reason for hiding this comment

ahrtr Sep 18, 2024

Choose a reason for hiding this comment

ahrtr commented Sep 18, 2024 • edited Loading

henrybear327 commented Sep 18, 2024

ivanvc commented Sep 18, 2024 • edited Loading

ahrtr commented Sep 18, 2024

ivanvc commented Sep 18, 2024 • edited Loading

ahrtr commented Sep 18, 2024

ahrtr left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 18, 2024

ahrtr commented Sep 18, 2024

ivanvc commented Sep 19, 2024 • edited Loading

ahrtr commented Sep 19, 2024

henrybear327 commented Sep 14, 2024 •

edited

Loading

henrybear327 commented Sep 14, 2024 •

edited

Loading

codecov-commenter commented Sep 14, 2024 •

edited

Loading

ivanvc commented Sep 17, 2024 •

edited

Loading

ahrtr commented Sep 18, 2024 •

edited

Loading

ivanvc commented Sep 18, 2024 •

edited

Loading

ivanvc commented Sep 18, 2024 •

edited

Loading

ivanvc commented Sep 19, 2024 •

edited

Loading