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.

Sign up for GitHub

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

Jump to bottom

go-mod: remove unecessary go module helper scripts #10221

Merged
merged 1 commit into from Feb 21, 2020
Merged

go-mod: remove unecessary go module helper scripts #10221

merged 1 commit into from Feb 21, 2020

Conversation

aanm
Copy link
Member

@aanm aanm commented Feb 17, 2020 •
edited

Scripts under contrib/go-mod don't provide any value to Cilium
repository as the dependencies are properly managed by go modules.

This commit removed all requires that are not necessary and left only
the needed requires. After removing non-essential requires go mod tidy && go mod vendor were executed to re-create the vendor directory with the
indirect dependencies resolved.

Signed-off-by: André Martins andre@cilium.io

This change is Reviewable

@aanm aanm added pending-review sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs release-note/misc This PR makes changes that have no direct user impact. labels Feb 17, 2020
@aanm aanm requested review from a team February 17, 2020 20:13
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Feb 17, 2020
@aanm aanm requested review from a team February 17, 2020 23:46
@aanm
Copy link
Member Author

aanm commented Feb 17, 2020

test-me-please

@aanm aanm added this to the 1.8 milestone Feb 18, 2020
@aanm aanm removed the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Feb 18, 2020
@aanm aanm requested a review from a team February 19, 2020 07:37
@aanm aanm requested a review from a team as a code owner February 19, 2020 07:37
@aanm aanm requested a review from a team February 19, 2020 07:37
@aanm aanm requested review from a team as code owners February 19, 2020 07:37
@aanm aanm requested a review from a team February 19, 2020 07:37
houndci-bot
houndci-bot reviewed Feb 19, 2020
View reviewed changes
pkg/option/config.go Outdated
case 0:
log.Warning("NodePort range was set but is empty.")
default:
return errors.New("Unable to parse min/max port value for NodePort range!")
Copy link

Choose a reason for hiding this comment

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

error strings should not be capitalized or end with punctuation or a newline

pkg/option/config.go Outdated
return fmt.Errorf("Unable to parse max port value for NodePort range: %s", err.Error())
}
if c.NodePortMax <= c.NodePortMin {
return errors.New("NodePort range min port must be smaller than max port!")
Copy link

Choose a reason for hiding this comment

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

error strings should not be capitalized or end with punctuation or a newline

houndci-bot
houndci-bot reviewed Feb 19, 2020
View reviewed changes
pkg/option/config.go Outdated
case 0:
log.Warning("NodePort range was set but is empty.")
default:
return errors.New("Unable to parse min/max port value for NodePort range!")
Copy link

Choose a reason for hiding this comment

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

error strings should not be capitalized or end with punctuation or a newline

pkg/option/config.go Outdated
return fmt.Errorf("Unable to parse max port value for NodePort range: %s", err.Error())
}
if c.NodePortMax <= c.NodePortMin {
return errors.New("NodePort range min port must be smaller than max port!")
Copy link

Choose a reason for hiding this comment

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

error strings should not be capitalized or end with punctuation or a newline

@aanm aanm changed the base branch from pr/drop-k8s-io-dependency to master February 19, 2020 07:38
@aanm
Copy link
Member Author

aanm commented Feb 19, 2020

test-me-please

@aanm
Copy link
Member Author

aanm commented Feb 19, 2020

test-missed-k8s

@aanm
Copy link
Member Author

aanm commented Feb 19, 2020

test-docs-please

@aanm
go-mod: remove unecessary go module helper scripts
0d5a193 
Scripts under contrib/go-mod don't provide any value to Cilium
repository as the dependencies are properly managed by go modules.

This commit removed all requires that are not necessary and left only
the needed requires. After removing non-essential requires `go mod tidy &&
go mod vendor` were executed to re-create the vendor directory with the
indirect dependencies resolved.

Signed-off-by: André Martins <andre@cilium.io>
@aanm aanm removed the sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. label Feb 21, 2020
@aanm
Copy link
Member Author

aanm commented Feb 21, 2020 •
edited

test-me-please https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/17373/ FQDN test

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 45.538% when pulling 0d5a193 on pr/remove-useless-requires into d8443c5 on master.

@aanm
Copy link
Member Author

aanm commented Feb 21, 2020

test-me-please

@aanm aanm merged commit f31485a into master Feb 21, 2020
1.8.0 automation moved this from In progress to Merged Feb 21, 2020
@aanm aanm deleted the pr/remove-useless-requires branch February 21, 2020 16:58
@aanm aanm mentioned this pull request Feb 21, 2020
Closed
@jrajahalme
Copy link
Member

jrajahalme commented Feb 21, 2020 •
edited

@aanm These changes look confusing, any idea why they were triggered here?

go.mod:

-	github.com/miekg/dns v1.1.4
+	github.com/miekg/dns v1.0.14
...
 	github.com/miekg/dns => github.com/cilium/dns v1.1.4-0.20190417235132-8e25ec9a0ff3

vendor/modules.txt:

-# github.com/miekg/dns v1.1.4 => github.com/cilium/dns v1.1.4-0.20190417235132-8e25ec9a0ff3
+# github.com/miekg/dns v1.0.14 => github.com/cilium/dns v1.1.4-0.20190417235132-8e25ec9a0ff3

@jrajahalme
Copy link
Member

One more note: I kind of liked the instructions on top of go.mod, could we have them back (updated to work without the scripts, of cause)?

@aanm
Copy link
Member Author

aanm commented Feb 21, 2020

@aanm These changes look confusing, any idea why they were triggered here?

go.mod:

-	github.com/miekg/dns v1.1.4
+	github.com/miekg/dns v1.0.14
...
 	github.com/miekg/dns => github.com/cilium/dns v1.1.4-0.20190417235132-8e25ec9a0ff3

vendor/modules.txt:

-# github.com/miekg/dns v1.1.4 => github.com/cilium/dns v1.1.4-0.20190417235132-8e25ec9a0ff3
+# github.com/miekg/dns v1.0.14 => github.com/cilium/dns v1.1.4-0.20190417235132-8e25ec9a0ff3

@jrajahalme the miekg/dns was updated to that version because that's the minimal version of miekg/dns the direct dependencies require. For that particular dependency it does not matter because we are overwriting it with whichever version we have in github.com/cilium/dns

One more note: I kind of liked the instructions on top of go.mod, could we have them back (updated to work without the scripts, of cause)?

there are no more scripts. The instructions were updated in the dev documentation https://docs.cilium.io/en/latest/contributing/development/dev_setup/#add-update-a-golang-dependency

TLDR to add/update a new dependency one simply needs to do:

$ go get github.com/containernetworking/cni@v0.5.2
$ go mod tidy
$ go mod vendor
$ git add go.mod go.sum vendor/

@rolinh rolinh mentioned this pull request Feb 24, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@joestringer joestringer joestringer approved these changes

Assignees
No one assigned
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
Milestone
1.8
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@aanm @coveralls @jrajahalme @joestringer @houndci-bot