Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Correct (my own) spelling of integrations #2312

Merged
merged 4 commits into from
Jul 31, 2019
Merged

Correct (my own) spelling of integrations #2312

merged 4 commits into from
Jul 31, 2019

Conversation

squaremo
Copy link
Member

@squaremo squaremo commented Jul 30, 2019 •
edited
Loading

I fully expect this to lay waste to the build (on this branch), for a while.

dholbach
dholbach previously approved these changes Jul 30, 2019
View reviewed changes
Copy link
Member

@dholbach dholbach left a comment

Choose a reason for hiding this comment

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

Well spotted. 🥖

@squaremo squaremo force-pushed the build/intergrations branch from 4a5bf7c to af2302e Compare July 30, 2019 15:09
@squaremo
Copy link
Member Author

I did not expect this to pass CI without further intervention*. Now, I am deeply confused.

*It does not complete make check-generated locally, because the code-generator got by go mod makes code that's incompatible with the apimachinery library also got by go mod.

@squaremo squaremo force-pushed the build/intergrations branch 3 times, most recently from 20c060b to e2c0347 Compare July 30, 2019 15:45
@squaremo
Copy link
Member Author

BOOYAH! I have made it fail for the right reason:
https://circleci.com/gh/fluxcd/flux/7110

There are two problems with make check-generated:

  1. integrations was spelt incorrectly in the diff command, meaning any diffs in the actual package would not be detected
  2. k8s.io/code-generator puts its output in the GOPATH-mandated location -- it doesn't know about go mod. So all the generated files were off elsewhere in the filesystem, and no diffs were detected.

Having fixed those two problems, now we are at the thing that actually needs to be fixed:

a. code-generator at the version indicated in go.mod generates code that is not compatible with apimachinery from the version indicated in go.mod.

@squaremo squaremo force-pushed the build/intergrations branch from e2c0347 to 004d37f Compare July 30, 2019 16:01
@squaremo squaremo dismissed dholbach’s stale review July 31, 2019 10:16

This PR is about fixing the problems that arise when check-generated is fixed

@squaremo
Copy link
Member Author

I'm not sure it's possible to keep all of these things:

  • using client-go v11
  • using go modules
  • generating kubernetes client code

The version of k8s.io/code-generator that will produce code compatible with client-go v11 (roughly, that at the tag kubernetes-1.14.4) isn't kitted out for go mod. Attempting to run it from its pkg/mod location (as the current update_codegen.sh script does) won't work, because it wants to go install itself, and it can't write to its own directory (by design of go mod). Attempting to vendor it and run it from vendor/k8s.io/code-generator just doesn't work -- go mod vendor just doesn't put it in vendor, I don't know why.

@squaremo squaremo force-pushed the build/intergrations branch 6 times, most recently from cd51685 to 24d94dc Compare July 31, 2019 12:47
squaremo added 4 commits July 31, 2019 13:54
@squaremo
Correct (my own) spelling of integrations
f4c3597
@squaremo
Align GOPATH and checkout in CI
04f8549 
In general we use go mod(ules), so it doesn't matter where the code is
checked out. But: the script bin/helm/update_codegen.sh puts its
generated files in the location indicated by their package and
GOPATH. So the checkout and that location should be the same.
@squaremo
Pin k8s.io dependencies to kubernetes-1.14.4
fad5ff0 
The way to do this (I eventually figured out) is to use `replace`
lines in go.mod.  Just using `go get <whatever kubernetes-1.14.4`, or
putting the equivalent `require` line in go.mod, will usually result
in the head revision.
@squaremo
Copy codegen package locally so it can be compiled
3e935e8 
The k8s.io/code-generator package version needed to produce code
compatible with client-go@v11 is old enough that it won't work with
`go mod`. To be able to use it, we have to copy it locally and let it
be compiled there.
@squaremo squaremo force-pushed the build/intergrations branch from 24d94dc to 3e935e8 Compare July 31, 2019 12:54
@squaremo squaremo requested review from 2opremio and hiddeco July 31, 2019 13:15
hiddeco
hiddeco approved these changes Jul 31, 2019
View reviewed changes
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

I do not especially like what I see but I do deem it necessary.

(Good work getting to the bottom of this nevertheless, it must have been a bumpy ride 🥇)

@squaremo squaremo merged commit dc84956 into master Jul 31, 2019
@squaremo squaremo deleted the build/intergrations branch July 31, 2019 15:28
@squaremo
Copy link
Member Author

Thanks @hiddeco and @dholbach
Sorry to subject you to this necessary unpleasantness :-)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hiddeco hiddeco hiddeco approved these changes

@dholbach dholbach dholbach left review comments

@2opremio 2opremio Awaiting requested review from 2opremio

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@squaremo @dholbach @hiddeco