-
Notifications
You must be signed in to change notification settings - Fork 206
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
Generate patches for Kustomize to enable storage versioning #1973
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1973 +/- ##
==========================================
- Coverage 57.21% 57.16% -0.06%
==========================================
Files 422 425 +3
Lines 85624 85709 +85
==========================================
+ Hits 48993 48998 +5
- Misses 30368 30448 +80
Partials 6263 6263
Continue to review full report at Codecov.
|
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.
Looks mostly great! I think you need to build config/crd
rather than config/default
as it does too much (in particular name/namespace mangling), and then that has knock-on consequences for the tasks.
@@ -37,7 +37,7 @@ func createSharedEnvTest(cfg testConfig, namespaceResources *namespaceResources) | |||
environment := envtest.Environment{ | |||
ErrorIfCRDPathMissing: true, | |||
CRDDirectoryPaths: []string{ | |||
"../../config/crd/bases", | |||
"../../config/crd/packed", |
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.
Oh, I guess the benefit of putting the patched CRD output somewhere under config/crd
is that we can hardcode it here rather than trying to feed it in as a parameter somehow. That makes sense.
I've just realised - we'll also need a This tells kustomize how to find names and namespaces that need to be updated when it applies the mangling (to put Some documentation links about this: |
48edb50
to
9979cc3
Compare
/ok-to-test 9979cc3 |
@theunrepentantgeek I don't think you should need to ok-to-test this PR as it's not from a fork. Is that not what you experienced? |
3e618c7
to
c430dd5
Compare
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 looks good, but I'm not sure about copying the TypeMeta field - isn't that always determined by the package+type that it's on?
Taskfile.yml
Outdated
@@ -373,8 +374,17 @@ tasks: | |||
deps: [controller:generate-kustomize] | |||
dir: "v2/" | |||
cmds: | |||
- mkdir -p bin # in case it doesn’t exist | |||
- kustomize build config/default | sed -e 's@localhost:5000/azureserviceoperator:latest@{{.PUBLIC_REGISTRY}}{{.CONTROLLER_DOCKER_IMAGE}}@g' > bin/azureserviceoperator_{{.VERSION}}.yaml | |||
- mkdir -p bing # in case it doesn't exist |
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.
Uh, bing?
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 I fixed this with my force push. 😊
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 doesn't look fixed?
@@ -98,6 +107,9 @@ | |||
} | |||
destination.Status = status | |||
|
|||
// TypeMeta | |||
destination.TypeMeta = person.TypeMeta |
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 thought you needed to avoid doing this, since it ends up copying the GVK from one version to another?
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.
Yeah, I had changes that I hadn't pushed. Now fixed.
a0f7f6b
to
97bb1ed
Compare
f91dc34
to
f922a2e
Compare
62bfb54
to
e8ca5d3
Compare
/ok-to-test sha=fb727a1 |
What this PR does / why we need it:
Enabling versioning requires the yaml files generated by
controller-gen
to be modified by Kustomize to inject the required settings.We modify the existing
gen-kustomize
mode of our generator to write the required files.How does this PR make you feel:
![gif](https://camo.githubusercontent.com/84954b1fac3f29e3b1aa0ebb347fe5db29b4bdf5dd83d0b8d138e41b93c469fe/68747470733a2f2f6d656469612e67697068792e636f6d2f6d656469612f336f3771445958653051754c436d653146752f67697068792e676966)
Prerequisites: