-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
chore: Set Go mod to /v2 #4916
chore: Set Go mod to /v2 #4916
Conversation
Signed-off-by: Simon Behar <[email protected]>
Signed-off-by: Simon Behar <[email protected]>
module github.com/argoproj/argo/v2 | ||
|
||
go 1.13 | ||
|
||
replace github.com/argoproj/argo/v2 => ./ | ||
|
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.
This is a key change that simplifies the transition
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.
This is not a valid use of the replace directive. The result is that your packages are unusable outside of this module.
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.
You have to update all of the argo import paths in all source files and add a v2 component to them.
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.
Related: golang/go#34417
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.
You have to update all of the argo import paths in all source files and add a v2 component to them.
@peterbourgon I must be misunderstanding you, because we do update all the argo import paths and add a v2/
to them
Can you reach out to me on the ArgoProj slack so we can chat about this please? https://argoproj.github.io/community/join-slack I'm @simon
. I want to resolve this ASAP
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 followed the pattern used by etcd
(https://github.com/etcd-io/etcd/blob/a4fac14353e701ce78bc4ec6a31e4c923a9e3744/go.mod#L7) although their case seems to be a "repository of modules" instead of a single module, which could explain the difference. If the hesitation is the module referencing itself, then we could simply move all the code to a v2
folder as an easy change. All of the import paths already have v2
in them
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.
Oops! I somehow got a view of the PR/repo where the import statements weren't updated. But I see now that they are. Mea culpa for that bit. The issue with the replace directive is being addressed in #4959 👍
Signed-off-by: Simon Behar <[email protected]>
Signed-off-by: Simon Behar <[email protected]>
Signed-off-by: Simon Behar <[email protected]>
@@ -12,4 +12,5 @@ cat \ | |||
| sed 's/workflowtemplate\./io.argoproj.REPLACEME.v1alpha1./' \ | |||
| sed 's/workflow\./io.argoproj.REPLACEME.v1alpha1./' \ | |||
| sed 's/io.argoproj.REPLACEME.v1alpha1./io.argoproj.workflow.v1alpha1./' \ | |||
| sed 's/k8s.io./io.k8s./' | |||
| sed 's/k8s.io./io.k8s./' \ | |||
| sed 's/v1alpha1\.v1alpha1\./v1alpha1\./g' |
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.
Maybe remove
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 think the final swagger has some typo errors in it due to this file (not due to your changes, pre-existing).
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.
Will look
Signed-off-by: Simon Behar <[email protected]>
Signed-off-by: Simon Behar <[email protected]>
@@ -139,6 +139,8 @@ define protoc | |||
--grpc-gateway_out=logtostderr=true:${GOPATH}/src \ | |||
--swagger_out=logtostderr=true,fqn_for_swagger_name=true:. \ | |||
$(1) | |||
perl -i -pe 's|argoproj/argo/|argoproj/argo/v2/|g' `echo "$(1)" | sed 's/proto/pb.go/g'` |
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.
This might not be necessary after this PR is merged, but currently it's a chicken-and-egg problem
@@ -32,10 +32,10 @@ service InfoService { | |||
rpc GetInfo (GetInfoRequest) returns (InfoResponse) { | |||
option (google.api.http).get = "/api/v1/info"; |
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.
Should this now be v2
as well?
Signed-off-by: Simon Behar <[email protected]>
Closes: #4786
Checklist: