-
Notifications
You must be signed in to change notification settings - Fork 187
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
internal: add go.mod and go.sum file #111
Conversation
This commit was generated by go mod init && go mod tidy
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.
CL description: "internal" doesn't sound right. maybe "all" or just no prefix? |
@@ -0,0 +1,26 @@ | |||
cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw= |
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 this will cause circular imports? Do we need to split the cloud repo first...?
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.
Why does genproto need cloud.google.com/go?
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.
Yikes. Yeah. I'll go make the relevant mod files. The path is:
genproto -> grpc -> oauth2 -> cloud.google.com/go/compute/metadata
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've come to the conclusion that this cyclic dependency must be allowed if we want to add a go.mod file in the near future:
- Cyclic module dependencies are OK [1]. Cyclic package dependencies are not OK and enforced by the compiler.
- x/oauth2/google must get a mod file that specifically requires
cloud.google.com/go/compute/metadata
instead ofcloud.google.com/go
to resolve this circular dependency. - x/ repos are quite a ways out from getting modules [2] [3].
We could wait for the x/ changes, but I think given the first point it's OK to go ahead now. WDYT?
1: https://groups.google.com/forum/#!searchin/golang-nuts/import$20cycle%7Csort:date/golang-nuts/h2r8ktgOOZI/Pm6uyj5BCQAJ
2: golang/go#28136
3: golang/go#27858
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 slightly unfortunate but sounds fine with me.
Oh, that damn compute/metadata. That should definitely be its own module.
…On Tue, Nov 6, 2018 at 4:18 PM Jean de Klerk ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In go.sum
<#111 (comment)>:
> @@ -0,0 +1,26 @@
+cloud.google.com/go v0.26.0/go.mod h1:aQUYkXzVsufM+DwF1aE+0xfcU+56JwCaLick0ClmMTw=
Yikes. Yeah. I'll go make the relevant mod files. The path is:
genproto -> grpc -> oauth2 -> cloud.google.com/go/compute/metadata
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#111 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ARoHZQpK9EOcVUHtR1ORIDnDLB-Cp5fvks5usibhgaJpZM4YREwf>
.
|
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.
Please remove "internal" from description as @broady suggested, but LGTM.
This commit was generated by go mod init && go mod tidy