Bumped versions of k8s dependencies and controller-runtime and k8s dependencies

NikhilSharmaWe commented 2 years ago

What this PR does / why we need it:

This PR bumps versions of k8s dependencies and controller-runtime and k8s dependencies to be compatible with https://github.com/kubernetes-sigs/kubebuilder.

justinsb commented 2 years ago

Thanks @NikhilSharmaWe - looks like there are some breaking API changes in kubectl / cli-utils. I'll take a look!

cc @seans3

NikhilSharmaWe commented 2 years ago

@everettraven I also tried to make the code compatible with the changes made in the versions of kubectl and cli-runtime. But I was not able to find a proper approach. I tried to make some changes regarding the changes in the func Validator. Could you please take a look https://github.com/NikhilSharmaWe/kubebuilder-declarative-pattern/commit/b2da05f0abcd5d05d24726e39228f4b5d540fc6e.

After we can get a reasonable approach for func Validator, we can move to func NewApplyOptions.

everettraven commented 2 years ago

I also tried to make the code compatible with the changes made in the versions of kubectl and cli-runtime. But I was not able to find a proper approach. I tried to make some changes regarding the changes in the func Validator. Could you please take a look NikhilSharmaWe@b2da05f.

After we can get a reasonable approach for func Validator, we can move to func NewApplyOptions.

@NikhilSharmaWe Sure thing, I will take a look and see what I can come up with!

everettraven commented 2 years ago

@NikhilSharmaWe I made a draft PR so that I could have the tests run on any changes that I made. I was able to get the tests passing. You should be able to see the changes in the draft PR here: https://github.com/kubernetes-sigs/kubebuilder-declarative-pattern/pull/222

I wanted to let you be able to make the changes and have your PR merged in instead of mine since you have already done a bunch of great work on it. I hope the changes in that draft PR helps!

(I am also going to keep the draft open for now in case there is some need for more testing minor changes outside of this PR)

NikhilSharmaWe commented 2 years ago

@everettraven Thank you for showing the correct approach for resolving the breaking changes.

In pkg/patterns/declarative/pkg/applier/direct.go

For func NewFactory: After seeing your approach I think my approach was almost correct but I needed to resource.QueryParamFieldValidation instead of resource.QueryParamDryRun in func NewQueryParamVerifier.
For func NewApplyOptions: In the future are we thinking to improve this approach in the future by using the new functions added in the package k8s.io/kubectl/pkg/cmd/apply like https://pkg.go.dev/k8s.io/kubectl@v0.24.0/pkg/cmd/apply#ApplyFlags.ToOptions to produce the desired *apply.ApplyOptions instead of copying the function NewApplyOptions which was present in the pkg before. Or is this unnecessary?

c/c @camilamacedo86

everettraven commented 2 years ago

@NikhilSharmaWe

In pkg/patterns/declarative/pkg/applier/direct.go

* For func `NewFactory`: After seeing your approach I think my approach was almost correct but I needed to `resource.QueryParamFieldValidation` instead of `resource.QueryParamDryRun` in func  `NewQueryParamVerifier`.

I agree, I think your approach here was pretty much spot on and only required the change to use resource.QueryParamFieldValidation

* For func `NewApplyOptions`: In the future are we thinking to improve this approach in the future by using the new functions added in the package `k8s.io/kubectl/pkg/cmd/apply` like https://pkg.go.dev/k8s.io/kubectl@v0.24.0/pkg/cmd/apply#ApplyFlags.ToOptions to produce the desired `*apply.ApplyOptions` instead of copying the function `NewApplyOptions` which was present in the pkg before.
  Or is this unnecessary?

This is a great point. I think that this is definitely worth taking a look into and seeing if this would be a better solution than copying the NewApplyOptions from the previous implementation. I will take a look into this and see if I can come up with a logical way to do this.

everettraven commented 2 years ago

@NikhilSharmaWe I was able to get the apply options working using this snippet:

baseName := "declarative-direct"
applyFlags := apply.NewApplyFlags(f, ioStreams)
applyCmd := apply.NewCmdApply(baseName, f, ioStreams)
applyOpts, err := applyFlags.ToOptions(applyCmd, baseName, nil)
if err != nil {
    return fmt.Errorf("error getting apply options: %w", err)
}

It also required moving the factory creation outside of the if opt.Validate block like so:

f := cmdutil.NewFactory(&genericclioptions.ConfigFlags{})

if opt.Validate {
     // implementation ...
}

Thanks for pointing that out!

k8s-ci-robot commented 2 years ago

@everettraven: changing LGTM is restricted to collaborators

In response to [this](https://github.com/kubernetes-sigs/kubebuilder-declarative-pattern/pull/217#pullrequestreview-983754072): >Looks good to me! Great job @NikhilSharmaWe ! Not sure I have permissions for it, but: > >/lgtm >/approve Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

everettraven commented 2 years ago

@everettraven Thank you for considering the suggestion. Just wanted to ask how did found out that the base name should be declarative-direct.

It could be any string, but when looking into the details of the apply.NewCmdApply and Apply.ToOptions the baseName is used when printing messages or warnings (can be seen here: https://github.com/kubernetes/kubectl/blob/9a9633e4515045b45600d2e9cc5f0498c5a4d0d1/pkg/cmd/apply/apply.go#L301-L303).

I felt like declarative-direct was descriptive enough to point it to this repo and being in the direct.go file that the setup is being done in.

Now that I think about it, kubebuilder-declarative-applier-direct would be much more descriptive so feel free to change it to that if you would like, but I think the former is still rather descriptive.

camilamacedo86 commented 2 years ago

/lgtm

atoato88 commented 2 years ago

/lgtm

@justinsb Could you check this?

everettraven commented 2 years ago

@justinsb could you take a look at this?

We have a couple projects relying on this dependency to be upgraded to support k8s 1.24 before they can be upgraded.

Thanks!

everettraven commented 2 years ago

@atoato88 or @justinsb is there anything else that needs to be done to get this merged?

atoato88 commented 2 years ago

/approve Sorry for my delay. Thank you to create and update PR.

It already looks good for me, and I thinked it is also good to get review from other approver if possible. But I approve this because this PR is getting time.

k8s-ci-robot commented 2 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atoato88, camilamacedo86, everettraven, NikhilSharmaWe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubernetes-sigs/kubebuilder-declarative-pattern/blob/master/OWNERS)~~ [atoato88] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

jcanseco commented 2 years ago

FYI we started to encounter an issue that seems to have been introduced in this PR: https://github.com/kubernetes-sigs/kubebuilder-declarative-pattern/issues/224

Wondering if anyone can take a look.

kubernetes-sigs / kubebuilder-declarative-pattern

Bumped versions of k8s dependencies and controller-runtime and k8s dependencies #217