-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
✨ feat: accept resource mutators in Move operation #7966
✨ feat: accept resource mutators in Move operation #7966
Conversation
Hi @takirala. 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 Once the patch is verified, the new status will be reflected by the 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. |
/ok-to-test |
/ok-to-test |
The tests are failing with
I imagine this is expected as we need to change the interface. Any advice on how can I move forward or any other advice (accept the the breaking change VS making changes backwards compatibe - i don't know how though) cc @fabriziopandini @sbueringer ?! |
pull-cluster-api-apidiff-main is not a blocking job, it is just a warning for reviewers to look at what is changed in our public API |
Will we be able to release in a 1.3.x patch, given the clusterctl/client/cluster go API change? Or will this need to wait for the first 1.4 release? |
In general our policy today doesn't allow feature backports (so it has to wait even if it wouldn't change the API) |
Signed-off-by: Tarun Gupta Akirala <takirala@users.noreply.github.com>
8cea251
to
55fd1f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! I don't see anything that looks wrong on a first pass. Have you been able to try running this to move a cluster based on the use case you outlined?
Signed-off-by: Tarun Gupta Akirala <takirala@users.noreply.github.com>
@Jont828 yes! I have validated the following scenario: cluster A (
cluster B (
|
/test pull-cluster-api-test-mink8s-main |
LGTM label has been added. Git tree hash: b5c246c338f721cc3b0ed465b723e21fca1327ef
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will be happy to get this in this release if we can address comments before the code freeze (around the 15th of March, please check the exact date)
While reviewing the code with @takirala, we realized that there is a small bug here, namely that we call |
Signed-off-by: Tarun Gupta Akirala <takirala@users.noreply.github.com>
@fabriziopandini @ykakarap all the previous comments are addressed to the best of my understanding - could you PTAL again ? Thanks! |
Signed-off-by: Tarun Gupta Akirala <takirala@users.noreply.github.com>
Ping @fabriziopandini @ykakarap can you PTAL again ? TIA! |
@takirala: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
Ping @fabriziopandini @ykakarap can you PTAL again ? TIA! |
Great work, and sorry for the lack of answer but between KubeCon and other internal events April was mostly an AFK month for me. Unfortunately, it seems that both @ykakarap and @Jont828 didn't have the bandwidth to take a look at this PR too, but I think the implementation is sound and if they have some feedback there is room to get them addressed before the release. |
LGTM label has been added. Git tree hash: 7fd99f31dcdf3f58d85ac7ee6b2068ae663cedda
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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 |
What this PR does / why we need it:
As a developer I would like to move capi resources from one cluster to another cluster and while doing so, i would want to control the namespace the resources are created in the target cluster. Right now, when i perform a move operation, clusters are always moved to the same namespace on target cluster as they are originally created in the originating cluster. I want to be able to control this so that I can move resources from multiple clusters to a single cluster and still organize them by namespace post move operation.
This PR takes the approach discussed in #7940 (comment) An alternative to this PR was initially proposed at #7941 but that approach seemed inflexible. So this PR overrides #7941
Which issue(s) this PR fixes:
Fixes #7940