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

fix!: update crd to apiextensions.k8s.io/v1 #681

Merged
merged 17 commits into from May 10, 2021
Merged

fix!: update crd to apiextensions.k8s.io/v1 #681

merged 17 commits into from May 10, 2021

Conversation

Flydiverny
Copy link
Member

@Flydiverny Flydiverny commented Mar 29, 2021

Fixes #680, fixes #645, fixes #621, closes #377

  • Drops support for secretDescriptor in CRD validation (its been deprecated forever, wasn't really validated before either but seemed to work regardless)
  • Updates to apiextensions.k8s.io/v1 for CRD
  • Updated validation schema to comply with structural requirements 😄
  • If the schema is missing anything that was used those fields will be dropped as soon as the CRD is updated! (setting preserveUnknownFields: true is not allowed)

This shouldn't be a breaking change for users as long as the validation schema includes all the possible props. I've gone thru the backends specOptions and keyOptions and I believe I've caught them all.. (assuming no one uses secretDescriptor)

I suppose this also drops support for kubernetes versions <1.16

@Flydiverny Flydiverny changed the title Update crd fix!: update crd to apiextensions.k8s.io/v1 Mar 29, 2021
@Flydiverny Flydiverny requested a review from moolen March 29, 2021 05:09
type: string
data:
type: array
items:
Copy link
Member Author

Choose a reason for hiding this comment

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

we could set x-kubernetes-preserve-unknown-fields: true to allow extra fields here

description: Template which will be deep merged without mutating
any existing fields. into generated secret, can be used to
set for example annotations or type on the generated secret
spec:
Copy link
Member Author

Choose a reason for hiding this comment

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

we could set x-kubernetes-preserve-unknown-fields: true to allow extra fields here

@moolen
Copy link
Member

moolen commented Mar 31, 2021

Hey @Flydiverny im on vacation for the next ~2 Weeks, i'll take a look at when i'm back!

Copy link
Member

@moolen moolen left a comment

Choose a reason for hiding this comment

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

Hey @Flydiverny, i'm back from vacation, i'll put my hands on it later today. I found a small issue that prevents e2e tests from running properly.

type: object
required:
- spec
Copy link
Member

Choose a reason for hiding this comment

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

The required field is defined twice (that's why the e2e test fails)

Copy link
Member Author

Choose a reason for hiding this comment

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

Guess I failed rebase after all 😸 ill see if I get some time to incorporate latest additions as well

@moolen
Copy link
Member

moolen commented Apr 14, 2021

LGTM! I tested it locally - e2e tests run fine, i can install all provided examples and i double-checked spec/key opts.

I think i would not set x-kubernetes-preserve-unknown-fields=true on spec/key opts to reject invalid resources. If a field is missing let's add it to the CRD.

Signed-off-by: Markus Maga <markus@maga.se>
…: false

Signed-off-by: Markus Maga <markus@maga.se>
Signed-off-by: Markus Maga <markus@maga.se>
Signed-off-by: Markus Maga <markus@maga.se>
Signed-off-by: Markus Maga <markus@maga.se>
Signed-off-by: Markus Maga <markus@maga.se>
Signed-off-by: Markus Maga <markus@maga.se>
Signed-off-by: Markus Maga <markus@maga.se>
Signed-off-by: Markus Maga <markus@maga.se>
Signed-off-by: Markus Maga <markus@maga.se>
Signed-off-by: Markus Maga <markus@maga.se>
Signed-off-by: Markus Maga <markus@maga.se>
Signed-off-by: Markus Maga <markus@maga.se>
Signed-off-by: Markus Maga <markus@maga.se>
Signed-off-by: Markus Maga <markus@maga.se>
Signed-off-by: Markus Maga <markus@maga.se>
Signed-off-by: Markus Maga <markus@maga.se>
@Flydiverny
Copy link
Member Author

Finally got around to rebasing this.
controllerId added from master changes and duplicate required: - spec bit removed.

@Flydiverny Flydiverny marked this pull request as ready for review May 9, 2021 17:55
@Flydiverny Flydiverny requested a review from moolen May 9, 2021 17:56
Copy link
Member

@moolen moolen left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

I tested the upgrade path and it worked seamlessly

@Flydiverny Flydiverny merged commit 73aeaef into master May 10, 2021
@Flydiverny Flydiverny deleted the update-crd branch May 10, 2021 20:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants