Skip to content
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

Revert "Simplified and updated go.mod" #1326

Closed
wants to merge 1 commit into from

Conversation

porridge
Copy link
Member

@porridge porridge commented Feb 3, 2020

Reverts #1325 because it re-introduces the issue fixed in #1244
cc @armandgrillet @nfnt

k8s.io/apiextensions-apiserver v0.0.0-20191016113550-5357c4baaf65
k8s.io/apimachinery v0.0.0-20191028221656-72ed19daf4bb
k8s.io/client-go v11.0.0+incompatible
k8s.io/code-generator v0.18.0-alpha.1.0.20191220033320-6b257a9d6f46
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is only about the code-generator, let's only bump this to v0.18.0-alpha, not revert the bump of the other deps to v0.16.6. Bumping the other deps fixes issues as well.

@nfnt
Copy link
Member

nfnt commented Feb 3, 2020

How does the issue in #1244 manifest? It isn't detected by the CI, which makes it hard to determine which dependencies can be bumped. How about adding a replace directive in go.mod instead? This would make it clear that this specific version is intended to be used. Not sure if comments can be used in go.mod, but having an additional comment here would be great as well.

@armandgrillet
Copy link
Contributor

Sorry for reintroducing a bug, I was unaware of the mentioned issue. I've just tried bumping k8s.io/code-generator on the master branch to v0.18.0-alpha.1 (then v0.18.0-alpha.2) but it then introduces other issues, will have to keep working on it.

@porridge
Copy link
Member Author

porridge commented Feb 3, 2020

How does the issue in #1244 manifest?

Code generation script barfs when running with GOPATH unset or GOBIN set to something else than $GOPATH/bin, IIRC.

It isn't detected by the CI, which makes it hard to determine which dependencies can be bumped.

Right. I don't think we invoke code generation in the CI runs at all, do we?

How about adding a replace directive in go.mod instead? This would make it clear that this specific version is intended to be used.

When reading the documentation for go.mod it did not seem to me that this is what replace is to be used for? Also, I'm not sure that:

require k8s.io/code-generator v0.16.6
replace k8s.io/code-generator v0.16.6 => k8s.io/code-generator v0.18.whatever

would qualify as making the intent clear :-)

Not sure if comments can be used in go.mod, but having an additional comment here would be great as well.

I think it allows comments. However I don't think that would scale. Even if we commented that a particular version of a dependency is necessary for some reason X, we would have no idea whether that ceases to be true if X is no longer a valid reason (as there can be other reasons).

This whole file is a delicate mesh of subtleties and it's not possible to document what would fail in case one of these lines are changed. The only assertion we can make is that "it worked like this for whoever touched it last, possibly only after lots of sweat and tears". I think that increasing test coverage to also invoke the less often used tooling is the only way to go...

@armandgrillet
Copy link
Contributor

@porridge Would backporting kubernetes/code-generator@6b257a9 to https://github.com/kubernetes/code-generator/tree/release-1.16 and then using it in KUDO make sense?

@porridge
Copy link
Member Author

porridge commented Feb 4, 2020

@porridge Would backporting kubernetes/code-generator@6b257a9 to https://github.com/kubernetes/code-generator/tree/release-1.16 and then using it in KUDO make sense?

In theory yes, but in practice it took over a month to get that change merged, and this time we'd need to wait for another k8s point release after that.
But perhaps we could replace k8s.io/code-generator v0.16.6 => github.com/kudobuilder/code-generator v0.16.6-withpatch
WDYT about creating a temporary fork under kudobuilder @gerred ?

@jimmidyson
Copy link

jimmidyson commented Feb 4, 2020

If code-generator is only used as a tool (not runtime dependency) then I would consider making tools a go module in it's own right in a subdirectory (/tools/) in this repo. Sorry if I've missed context!

@gerred
Copy link
Member

gerred commented Feb 19, 2020

@jimmidyson you're not wrong. Check out tools.go - that's how and why we're getting these into go.mod, based on the recommendation here: https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module

It's a little clunky, but it's at least recognizable as a pattern in a few projects now. @porridge I'm OK with that patch. Do we have a path forward to not rely on that long term and get these dependencies up to date? Or at least, a diff of things that prevent us from using the most up to date code generator now? If so, we can in parallel start working on stamping out those issues in the KUDO codebase proper.

@jimmidyson
Copy link

@gerred My point is that by adding all the tools dependencies into your module's go mod you're adding dependencies which aren't actually source dependencies, but purely build dependencies. By moving the tools dependencies into their own submodule in <REPO_ROOT>/tools then you separate out build time dependencies from source dependencies, which has saved me time on other projects when updating conflicting dependencies, as well as benefitting downstream projects by limiting transitive dependencies, again to save time fighting conflicting dependencies.

porridge added a commit that referenced this pull request Feb 21, 2020
This is implementing a workaround as discussed in
#1326

The logic in update-codegen.sh is changed to deal with the replace.

We can just remove the replace once we upgrade to upstream
code-generator which has the change, i.e.
v0.18.0-alpha.1.0.20191220033320-6b257a9d6f46 or later.

Signed-off-by: Marcin Owsiany <mowsiany@D2iQ.com>
porridge added a commit that referenced this pull request Feb 28, 2020
This is implementing a workaround as discussed in
#1326

The logic in update-codegen.sh is changed to deal with the replace.

We can just remove the replace once we upgrade to upstream
code-generator which has the change, i.e.
v0.18.0-alpha.1.0.20191220033320-6b257a9d6f46 or later.

Signed-off-by: Marcin Owsiany <mowsiany@D2iQ.com>
Co-authored-by: Andreas Neumann <aneumann@mesosphere.com>
@porridge
Copy link
Member Author

Superseded by #1359

@porridge porridge closed this Feb 28, 2020
@porridge porridge deleted the revert-1325-D2IQ-62637 branch February 28, 2020 10:45
runyontr pushed a commit that referenced this pull request Mar 11, 2020
This is implementing a workaround as discussed in
#1326

The logic in update-codegen.sh is changed to deal with the replace.

We can just remove the replace once we upgrade to upstream
code-generator which has the change, i.e.
v0.18.0-alpha.1.0.20191220033320-6b257a9d6f46 or later.

Signed-off-by: Marcin Owsiany <mowsiany@D2iQ.com>
Co-authored-by: Andreas Neumann <aneumann@mesosphere.com>
Signed-off-by: Thomas Runyon <runyontr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants