-
Notifications
You must be signed in to change notification settings - Fork 63
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
move to go.mod #47
move to go.mod #47
Conversation
Hi @sanchezl. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
Why are there changes in the vendor/ folder? I expect with #41 already merged, this PR will not change the vendor/ folder. |
|
/ok-to-test |
go.mod
Outdated
k8s.io/kube-aggregator v0.0.0-kubernetes-1.16.2 | ||
) | ||
|
||
replace ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the "replace" section manually edited? Why are the specific revisions used here, instead of v0.0.0-kubernetes-1.16.2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the replace
section is manually edited.
go.mod
does not allow you to specify a branch or non-semver tag. If you are not using a semver tag (e.g. v1.0.0
), which kubernetes does not at the moment, you must use a revision, and that is converted to a pseudo-version. Unfortunately, when you run certain go
commands (such as go build
), there is a chance that the pseudo version will be replaced with the latest from master
. So...
- I set the
k8s.io/*
dependencies to a pseudo version ofv0.0.0-kubernetes-1.16.2
. The format accomplishes two things: 1.go
sees it as a pseudo-version and 2. thekubernetes-1.16.2
part is a reminder of what I really want to require. - Adding
replace
entries fork8s.io/*
entries accomplishes the following: 1. the psedu-version in therequire
section is ignored, and 2. The revision of thek8s.io/*
module is effectively pinned and won't change unless explicitly updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation.
If making "v0.0.0-kubernetes-1.16.2" a pseudo version isn't a common practice, can you document the purpose in comments?
In the future, when we need to bump the version of k8s.io/*, I think we need to manually edit the "require" section and the "replace" section, right?
Unfortunately, when you run certain go commands (such as go build), there is a chance that the pseudo version will be replaced with the latest from master
Can you say what commands exactly?
Anyhow, if we just use the revision based pseudo-version generated by go
, it's not obvious what tag the revision matches to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do the 1.17 port soon. It has proper semver tags.
@@ -0,0 +1,34 @@ | |||
module github.com/kubernetes-sigs/kube-storage-version-migrator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch to "sig.k8s.io"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do that in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sanchezl it's a small thing and it's been two months.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cbf34b9
to
3bfeaef
Compare
/test pull-kube-storage-version-migrator-disruptive |
1063503
to
42d6edb
Compare
3956ca8
to
c3307cd
Compare
/retest |
1 similar comment
/retest |
ae4e310
to
657a724
Compare
/test pull-kube-storage-version-migrator-disruptive |
/test pull-kube-storage-version-migrator-disruptive |
@sanchezl thanks for the investigation. Can you say more about this bug? Is it stuck at this line? Is the storage migrator handling a k8s native resource or a CRD? |
This particular bug does not happen in any of the tests in this repo. I found it when testing on my own clusters and accidentally providing invalid GVRs. |
d031dbe
to
1abfac2
Compare
77bd1ac
to
7b62c96
Compare
/retest |
1 similar comment
/retest |
@caesarxuchao , @deads2k PTAL, I've finally gotten the tests to run consistently. |
/retest |
pkg/controller/kubemigrator.go
Outdated
metrics.Metrics.ObserveFailedMigration(resource(m).String()) | ||
return err | ||
klog.Errorf("%v: migration failed: %v", m.Name, err) | ||
return updateErr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thi sis a change in behavior. Why shouldn't we return the failure when we failed. This allows us to return nil after having an error. If this is correct, add comment explaining why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did not intend to change behavior. Any recommendations for handling the updateErr
?
Please comment at #58 instead.
d7f59b4
to
3519eed
Compare
We import sigs.k8s.io/structured-merge-diff via versioned imports. This breaks the use of dep. We need this now. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: caesarxuchao, deads2k, sanchezl, sttts 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:
Approvers can indicate their approval by writing |
go mod init
go mod vendor