-
Notifications
You must be signed in to change notification settings - Fork 174
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
switch dep to go mod and upgrade go version #67
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
module sigs.k8s.io/kubernetes-sigs/sig-storage-lib-external-provisioner/examples/hostpath-provisioner | ||
|
||
go 1.13 | ||
|
||
require ( | ||
github.com/miekg/dns v1.1.27 // indirect | ||
github.com/prometheus/client_golang v1.4.1 // indirect | ||
golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d // indirect | ||
golang.org/x/time v0.0.0-20191024005414-555d28b269f0 // indirect | ||
k8s.io/api v0.17.3 | ||
k8s.io/apimachinery v0.17.3 | ||
k8s.io/client-go v0.0.0-20200131194156-19522ff28802 // release-14.0 | ||
k8s.io/klog v1.0.0 | ||
k8s.io/utils v0.0.0-20200124190032-861946025e34 // indirect | ||
sigs.k8s.io/sig-storage-lib-external-provisioner v4.0.1+incompatible | ||
) |
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
module sigs.k8s.io/sig-storage-lib-external-provisioner | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe the next release with this change will have to be v5.0.0 because introducing Go module support is a breaking change. Also, because the major version is > 1, versioned imports become mandatory. This means that This also raises the question whether importing with dep is still supposed to work. It doesn't out-of-the-box anymore because dep doesn't understand versioned imports. We worked around that with symlinks elsewhere, see kubernetes-csi/external-attacher#209 and kubernetes-csi/csi-test#232. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a single commit is added to fix this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The commit message of that commit ("add compatibility with dep") misses the main reason for doing this: "go mod" will also refuse to pull the code unless the /v5 suffix matches the major version number. Not sure whether you want to fix that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. git message amended. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I probably explained this wrong 😁 The "/v5" suffix isn't needed by dep, quite the opposite, it just gets confused by it. It's needed by "go mod". Anyway, let's leave the commit message as-is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, I'm new to this pattern. I just read https://blog.golang.org/v2-go-modules and in their example they don't symlink, instead they keep v0/v1 in the top-level and they let all the versions coexist in the same branch. I don't want to follow that example because this library makes breaking changes quite often and it would be a pain to retroactively make v2-v5 directories. But I think symlinking could get pretty tedious as well. If I want to refactor and add/delete a file, I need to make sure the symlink under /v5 is updated accordingly? Is there a common script all our repos could share that could make validation & maintenance of these symlinks easier? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens when I want to release a v6? Would I simply rename v5 to v6? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree, having multiple different versions in the same branch looks awkward. It's probably only relevant if developers really need to support multiple API versions. Symlinking is a stop-gap measure to continue supporting dep. I don't know how important that is for the consumers of this library. It is something that could be scripted, but there is no script at the moment.
Yes. |
||
|
||
go 1.13 | ||
|
||
require ( | ||
github.com/miekg/dns v1.1.27 | ||
github.com/onsi/ginkgo v1.12.0 // indirect | ||
github.com/onsi/gomega v1.9.0 // indirect | ||
github.com/prometheus/client_golang v1.4.1 | ||
golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d // indirect | ||
golang.org/x/time v0.0.0-20191024005414-555d28b269f0 | ||
k8s.io/api v0.17.3 | ||
k8s.io/apimachinery v0.17.3 | ||
k8s.io/client-go v0.0.0-20200131194156-19522ff28802 // release-14.0 | ||
k8s.io/klog v1.0.0 | ||
k8s.io/utils v0.0.0-20200124190032-861946025e34 // indirect | ||
) |
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.
Can this be enhanced to get a fixed version of
gometalinter
while you are touching this code? Like depending on "master" for Kubernetes, depending on master of gometalinter (or golangci-lint) has the risk that changes in those project(s) break this project.I know that this is more complicated because (for reasons that I don't understand)
go get
in non-module mode doesn't support specifying a version, but perhaps it works when adding it as module and then building?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.
with GO111MODULE=off its not easy, but we can fix it later by replacing with golangci-lint. In fact gometalinter is DEPRECATED, so its 'fixed'.