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

Move KUDO to v1 CRDs #1725

Merged
merged 6 commits into from
Nov 12, 2020
Merged

Move KUDO to v1 CRDs #1725

merged 6 commits into from
Nov 12, 2020

Conversation

alenkacz
Copy link
Contributor

What this PR does / why we need it:
This moves KUDO to v1 CRDs only (meaning we support only k8s 1.16+).

Fixes #1723

@alenkacz alenkacz changed the title Move KUDO to v1 CRDs WIP: Move KUDO to v1 CRDs Oct 21, 2020
@alenkacz alenkacz force-pushed the av/v1crd branch 3 times, most recently from 0ff713d to 7c357c2 Compare October 21, 2020 11:34
Signed-off-by: Alena Varkockova <varkockova.a@gmail.com>
Signed-off-by: Alena Varkockova <varkockova.a@gmail.com>
Signed-off-by: Alena Varkockova <varkockova.a@gmail.com>
Signed-off-by: Alena Varkockova <varkockova.a@gmail.com>
Signed-off-by: Alena Varkockova <varkockova.a@gmail.com>
@alenkacz alenkacz changed the title WIP: Move KUDO to v1 CRDs Move KUDO to v1 CRDs Nov 10, 2020
Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

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

Removes the kube 1.18 warning messages for CRD 👍
Leaves the warning for admissionregistration.k8s.io/v1beta1 MutatingWebhookConfiguration

LGTM!

Left some thoughts / questions but nothing that should stop this from landing!

@@ -25,7 +25,7 @@ import (
"strings"
"time"

apiextenstionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
apiextenstionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
Copy link
Member

Choose a reason for hiding this comment

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

not an issue... but we have this defined in other places

  1. extv1
  2. apiext1 (which is my preference)

we should standard it. (not necessarily part of this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

went for apiext1 thanks

@@ -7,6 +7,8 @@ import (
"fmt"
"log"

apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
Copy link
Member

Choose a reason for hiding this comment

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

we should be consistent tho...

@@ -1,5 +1,3 @@
// +build integration

package cmd
Copy link
Member

Choose a reason for hiding this comment

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

is there a reason to remove +build integration tag? Should the class be renamed to reflect it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, thank you

@@ -63,7 +63,8 @@ func paramDefaults(pf *packages.Files) verifier.Result {
res := verifier.NewResult()
for _, p := range pf.Params.Parameters {
if p.HasDefault() {
if err := p.ValidateDefault(); err != nil {
if err := p.
Copy link
Member

Choose a reason for hiding this comment

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

ugh... do we really need to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, I don't know what happened here :/

Signed-off-by: Alena Varkockova <varkockova.a@gmail.com>
@alenkacz alenkacz merged commit 287faea into main Nov 12, 2020
@alenkacz alenkacz deleted the av/v1crd branch November 12, 2020 08:05
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.

Support both v1 and v1beta1 crds
2 participants