Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run test-go-mod on local modules #4975

Closed

Conversation

annasong20
Copy link
Contributor

@annasong20 annasong20 commented Jan 12, 2023

This PR comments out the test-go-mod prow-presubmit job.

This PR changes test-go-mod so that go mod tidy is run with local module dependencies instead of the latest released modules. This makes the CI much friendlier to changes that across multiple modules.

The failed job as a result of cross-module changes in #4959 motivated this change.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 12, 2023
@annasong20
Copy link
Contributor Author

/cc @natasha41575

go.work.sum Outdated Show resolved Hide resolved
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: annasong20, yuwenma
Once this PR has been reviewed and has the lgtm label, please assign knverey for approval by writing /assign @knverey in a comment. 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:

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

@yuwenma
Copy link
Contributor

yuwenma commented Jan 13, 2023

/lgtm

cc @koba1t who made the go workspace improvements for kustomize.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 13, 2023
@annasong20
Copy link
Contributor Author

/cc @koba1t

@k8s-ci-robot k8s-ci-robot requested a review from koba1t January 13, 2023 17:16
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2023
Makefile Outdated Show resolved Hide resolved
@k8s-ci-robot
Copy link
Contributor

This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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/test-infra repository.

@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 24, 2023
@annasong20
Copy link
Contributor Author

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jan 24, 2023
@annasong20 annasong20 changed the title Remove go module presubmit job Run test-go-mod on local modules Jan 25, 2023
Copy link
Contributor Author

@annasong20 annasong20 left a comment

Choose a reason for hiding this comment

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

The test-go-mod rule should now add replace statements, run go mod tidy, then remove said replace statements on all modules.

I left my concerns below.

Comment on lines +5 to +6
read -a modules <<< $(go list -m)
read -a module_paths <<< $(go list -m -f {{.Dir}})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to move this into Makefile to avoid re-calculating it for each module, but I don't know how to export variables in Makefile so that they're accessible here.

Comment on lines +5 to +6
read -a modules <<< $(go list -m)
read -a module_paths <<< $(go list -m -f {{.Dir}})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm assuming go list -m returns the modules in the same order on every call.

I've considered 2 alternatives:

  • go list -m -json returns all information, including name and directory path, for all modules, but I'd have to parse the json output
  • fetch the module name as I navigate to each module with hack/for-each-module.sh; however, go list -m lists all modules in the workspace regardless of the working directory, so the best I could come up with was parsing the go.mod file for the module name

Comment on lines +155 to +157
./hack/for-each-module.sh $$(pwd)/hack/replace.sh; \
./hack/for-each-module.sh "go mod tidy -v"; \
./hack/for-each-module.sh $$(pwd)/hack/dropReplace.sh
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried inlining the scripts in replace.sh and dropReplace.sh, but couldn't figure out how to escape the relevant characters.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think calling a script from here is better than inlining the details anyway. :)

I'd suggest making a single script we can invoke from here like ./hack/for-each-module.sh "\$$KUSTOMIZE_ROOT/hack/go-mod-tidy-kust-dev.sh" and then adding a bunch of comments in that file to explain the steps and why we're doing them. Or to be more flexible, we can make a script that itself takes a command to run in the context of the "pinned" module, invoked like: ./hack/for-each-module.sh "\$$KUSTOMIZE_ROOT/hack/with-pinned-kust-dev.sh 'go mod tidy -v'".

@@ -152,7 +152,9 @@ functions-examples-all:
done

test-go-mod:
./hack/for-each-module.sh "go list -m -json all > /dev/null && go mod tidy -v"
./hack/for-each-module.sh $$(pwd)/hack/replace.sh; \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried using $$KUSTOMIZE_ROOT instead of $$(pwd), but couldn't figure out how to use the exported hack/for-each-module.sh variable here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think "\$$KUSTOMIZE_ROOT/hack/... should work!

@natasha41575
Copy link
Contributor

natasha41575 commented Jan 31, 2023

Closing because #5011 will merge soon

@annasong20 annasong20 deleted the silence-module-job branch January 31, 2023 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants