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

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

Merged

Conversation

NikhilSharmaWe
Copy link
Member

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.

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 12, 2022
@justinsb
Copy link
Contributor

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

cc @seans3

go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
@NikhilSharmaWe

This comment was marked as outdated.

@everettraven

This comment was marked as resolved.

@everettraven

This comment was marked as resolved.

@NikhilSharmaWe NikhilSharmaWe force-pushed the bumpControllerRuntime branch 3 times, most recently from 5d2c99e to bc4b28e Compare May 24, 2022 16:43
@NikhilSharmaWe

This comment was marked as resolved.

@everettraven

This comment was marked as resolved.

@everettraven

This comment was marked as resolved.

@NikhilSharmaWe NikhilSharmaWe force-pushed the bumpControllerRuntime branch 2 times, most recently from ddfe740 to 1b4b7a0 Compare May 24, 2022 18:56
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 24, 2022
@everettraven everettraven mentioned this pull request May 24, 2022
@NikhilSharmaWe NikhilSharmaWe force-pushed the bumpControllerRuntime branch 2 times, most recently from d7b9a74 to 8cdf972 Compare May 24, 2022 19:22
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 24, 2022
Copy link
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

Looks good to me! Great job @NikhilSharmaWe ! Not sure I have permissions for it, but:

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

@everettraven: changing LGTM is restricted to collaborators

In response to this:

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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@everettraven
Copy link
Contributor

@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.

Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Great 👍
Nice contribution @NikhilSharmaWe and @everettraven 🥇

We need to ping the Owners of this repo so that they can help us move forward.

@justinsb

@camilamacedo86
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 24, 2022
@atoato88
Copy link
Contributor

/lgtm

@justinsb
Could you check this?

@everettraven
Copy link
Contributor

@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
Copy link
Contributor

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

@atoato88
Copy link
Contributor

atoato88 commented Jun 2, 2022

/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
Copy link
Contributor

[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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 2, 2022
@k8s-ci-robot k8s-ci-robot merged commit fe5be94 into kubernetes-sigs:master Jun 2, 2022
@jcanseco
Copy link
Contributor

jcanseco commented Jun 9, 2022

FYI we started to encounter an issue that seems to have been introduced in this PR: #224

Wondering if anyone can take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants