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.

Sign up for GitHub

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

Jump to bottom

✨ Introduce ClusterName field to ClusterResourceSetBinding #7680

Merged
merged 1 commit into from
Mar 10, 2023
Merged

✨ Introduce ClusterName field to ClusterResourceSetBinding #7680

merged 1 commit into from
Mar 10, 2023

Conversation

chaunceyjiang
Copy link
Contributor

@chaunceyjiang chaunceyjiang commented Dec 2, 2022 •
edited
Loading

Signed-off-by: chaunceyjiang [email protected]

What this PR does / why we need it:

Introduce ClusterName field to ClusterResourceSetBinding.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Part of #7669

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 2, 2022
@k8s-ci-robot
Copy link
Contributor

Welcome @chaunceyjiang!

It looks like this is your first PR to kubernetes-sigs/cluster-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @chaunceyjiang. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 2, 2022
@chaunceyjiang
Copy link
Contributor Author

/assign @killianmuldoon

killianmuldoon
killianmuldoon approved these changes Dec 2, 2022
View reviewed changes
Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

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

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 2, 2022
@killianmuldoon
Copy link
Contributor

Thanks for this @chaunceyjiang! Out of curiosity - what does "Incubation" mean in the title?

@chaunceyjiang chaunceyjiang changed the title ✨ Incubation ClusterName field to ClusterResourceSetBinding ✨ Introduce ClusterName field to ClusterResourceSetBinding Dec 2, 2022
@chiukapoor
Copy link
Contributor

Fixes #7669

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 2, 2022
@chaunceyjiang
Copy link
Contributor Author

Hi, @killianmuldoon
Just a question, does the v1alpha4. ClusterResourceSetBinding and v1alpha3. ClusterResourceSetBinding need the ClusterName field?

@killianmuldoon
Copy link
Contributor

Just a question, does the v1alpha4. ClusterResourceSetBinding and v1alpha3. ClusterResourceSetBinding need the ClusterName field?

Nope - this should only be added to the latest API types i.e. v1beta1. I think you need to run make generate and add the generated files including conversion to the PR to get the tests to run.

@chaunceyjiang chaunceyjiang force-pushed the cluster_name branch 2 times, most recently from 9d29c3f to ab0083c Compare December 4, 2022 09:58
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 4, 2022
@chaunceyjiang chaunceyjiang force-pushed the cluster_name branch 2 times, most recently from 0cddfa2 to 092a9b1 Compare December 4, 2022 10:18
@chaunceyjiang
Copy link
Contributor Author

/retest

@chaunceyjiang
Copy link
Contributor Author

Hi, @killianmuldoon

Ready for review.

@sbueringer
Copy link
Member

/test pull-cluster-api-e2e-full-main

@chaunceyjiang
Copy link
Contributor Author

/cc @killianmuldoon @sbueringer @fabriziopandini
Please take a look.

@chaunceyjiang
0. Introduce ClusterName field to ClusterResourceSetBinding
f469853 
1. remove the Cluster owner reference from the ClusterResourceSetBinding.

2. update the clusterName field in the CRS controller to update existing CRSbindings with the new field.

3. adding a check in the validation webhook preventing the cluster name to be changed once it is set

Signed-off-by: chaunceyjiang <[email protected]>
@sbueringer
Copy link
Member

/test pull-cluster-api-e2e-full-main

@sbueringer
Copy link
Member

sbueringer commented Mar 9, 2023 •
edited
Loading

lgtm from my side pending green test result + answers from Fabrizio/Killian on this conversation: #7680 (comment)

@sbueringer
Copy link
Member

/retest
another one of the many unrelated flakes that we have

@sbueringer
Copy link
Member

/retest

killianmuldoon
killianmuldoon approved these changes Mar 10, 2023
View reviewed changes
Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

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

/lgtm

Will leave the final approval until there's feedback from @sbueringer on #7680 (comment)

I did want to say though - @chaunceyjiang this is really great work. Thanks for sticking with it! - I know it's taken a very long time to get to the finish line.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: b63035825452d20f7b5cab7b59c83958bdc19169

@sbueringer
Copy link
Member

100% agree. Thx for the patience!!

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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 Mar 10, 2023
@k8s-ci-robot k8s-ci-robot merged commit 899b324 into kubernetes-sigs:main Mar 10, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.4 milestone Mar 10, 2023
@chaunceyjiang chaunceyjiang mentioned this pull request Mar 12, 2023
Open
This was referenced Mar 15, 2023
Closed
Closed
@fabriziopandini fabriziopandini mentioned this pull request Mar 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbueringer sbueringer sbueringer left review comments

@killianmuldoon killianmuldoon killianmuldoon approved these changes

@fabriziopandini fabriziopandini Awaiting requested review from fabriziopandini

Assignees

@sbueringer sbueringer

@fabriziopandini fabriziopandini

@killianmuldoon killianmuldoon

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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
6 participants
@chaunceyjiang @k8s-ci-robot @killianmuldoon @chiukapoor @fabriziopandini @sbueringer