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

Add go modules verification and update scripts #3995

Closed
wants to merge 8 commits into from

Conversation

eloyekunle
Copy link
Contributor

@eloyekunle eloyekunle commented Jul 1, 2019

Closes #3993

Adds two new package.json scripts:

  1. check:backend:vendor
  2. fix:backend:vendor

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 1, 2019
@k8s-ci-robot k8s-ci-robot requested review from cheld and PeWu July 1, 2019 13:30
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 1, 2019
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 1, 2019
@codecov
Copy link

codecov bot commented Jul 1, 2019

Codecov Report

Merging #3995 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3995      +/-   ##
==========================================
+ Coverage   47.27%   47.28%   +0.01%     
==========================================
  Files         182      182              
  Lines        8400     8400              
  Branches       71       71              
==========================================
+ Hits         3971     3972       +1     
+ Misses       4184     4183       -1     
  Partials      245      245
Impacted Files Coverage Δ
...p/backend/integration/metric/common/aggregation.go 90.9% <0%> (+1.81%) ⬆️

Continue to review full report at Codecov.

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

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 1, 2019
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 1, 2019
@eloyekunle eloyekunle changed the title WIP: Add go modules verification and update scripts Add go modules verification and update scripts Jul 1, 2019
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 1, 2019
@eloyekunle
Copy link
Contributor Author

/assign maciaszczykm floreks jeefy

@floreks
Copy link
Member

floreks commented Jul 1, 2019

@eloyekunle we do not need vendor directory in the dashboard repo anymore as go mod allows us to drop it. During default go build it is not used anyway. We would have to explicitly call go build -mod=vendor to use it. Required versions are defined in go.mod file. I have deleted this directory to make Dashboard repo smaller.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 1, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: eloyekunle
To complete the pull request process, please assign floreks
You can assign the PR to them by writing /assign @floreks in a comment when ready.

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

@eloyekunle
Copy link
Contributor Author

@floreks I have deleted the vendor/ directory from this PR and updated the scripts.

@shu-mutou
Copy link
Contributor

Cleaning up go.mod file using go mod tidy looks good for me. But could you let me know why we should keep vendor directory even if on local environment or travis. Can go mod not ensure library dependencies without running go mod vendor?

@floreks
Copy link
Member

floreks commented Jul 2, 2019

@shu-mutou Go docs only describe one reason to keep vendor directory. That is if user would use older Go version or have disabled go mod feature. Otherwise there is no benefit of keeping it inside the repo.

@shu-mutou
Copy link
Contributor

@floreks You mean we do not need to add go mod vendor into CI process, don't you? Our CI process does not seem to face any problem actually, so I agree with not to add go mod vendor.

@floreks
Copy link
Member

floreks commented Jul 2, 2019

@shu-mutou go mod vendor only creates a vendor directory and is not used by the go build by default. We can drop it everywhere.

go mod tidy -v

echo "vendor: running 'go mod vendor'"
go mod vendor
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed as it would create a local vendor dir.

@floreks
Copy link
Member

floreks commented Jul 2, 2019

@eloyekunle those vendor scripts are more kubernetes related. They won't really work for us since we will not be using vendor directory. All dependencies will be downloaded to your $GOPATH anyway. Also bumping and tidying Go deps is rather manual task for us. We do not need to do this on every npm run check. It should not be done silently in background as it might introduce some breaking changes.

@maciaszczykm WDYT?

@maciaszczykm
Copy link
Member

I agree. For me go mod is something like npm right now. It is a tool to be used directly. We don't have any scripts to update Node deps.

@eloyekunle
Copy link
Contributor Author

I agree, the scripts are unnecessary since we're not keeping the vendor directory anymore.

/close

@k8s-ci-robot
Copy link
Contributor

@eloyekunle: Closed this PR.

In response to this:

I agree, the scripts are unnecessary since we're not keeping the vendor directory anymore.

/close

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.

@eloyekunle eloyekunle deleted the feat/verify-vendor branch July 2, 2019 09:28
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verify vendored modules in pull requests
6 participants