switch dep to go mod and upgrade go version #67
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @u2takey! |
k8s-ci-robot
added
the
size/XL
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
| gometalinter --install | ||
| # todo gometalinter is DEPRECATED, Use https://github.com/golangci/golangci-lint | ||
| GO111MODULE=off go get -u github.com/alecthomas/gometalinter | ||
| GO111MODULE=off gometalinter --install |
There was a problem hiding this comment.
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.
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'.
go.mod
Outdated
| @@ -0,0 +1,17 @@ | |||
| module sigs.k8s.io/sig-storage-lib-external-provisioner | |||
There was a problem hiding this comment.
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 sigs.k8s.io/sig-storage-lib-external-provisioner has to be replaced with sigs.k8s.io/sig-storage-lib-external-provisioner/v5 here and in all import statements in the code itself.
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.
a single commit is added to fix this.
There was a problem hiding this comment.
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.
git message amended.
There was a problem hiding this comment.
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.
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.
What happens when I want to release a v6? Would I simply rename v5 to v6?
There was a problem hiding this comment.
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.
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.
What happens when I want to release a v6? Would I simply rename v5 to v6?
Yes.
|
@pohly: changing LGTM is restricted to collaborators In response to this:
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. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jsafrane, u2takey 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 |
k8s-ci-robot
added
the
approved
fix incorrect link
Fixes: #65