Skip to content
This repository has been archived by the owner on Jun 26, 2023. It is now read-only.

Add '--force' flag when switching from 'Ignore' to 'Propagate' mode #1169

Merged
merged 1 commit into from
Oct 6, 2020

Conversation

GinnyJI
Copy link
Contributor

@GinnyJI GinnyJI commented Oct 5, 2020

If the user switch the propagation mode directly from 'Ignore' to
'Propagate', it will fail with an error message telling the user that a
'--force' flag is needed. '-f' is also supported as a shorthand.

Tested: make test-e2e

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 5, 2020
@GinnyJI
Copy link
Contributor Author

GinnyJI commented Oct 5, 2020

/assign @adrianludwin
/assign @yiqigao217

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 5, 2020
@adrianludwin
Copy link
Contributor

/assign @yiqigao217

Yiqi, can you please work with Ginny to fine-tune the error message? We should explain exactly why this feature exists, and maybe link to some documentation as well (which we might need to write).

Copy link
Contributor

@adrianludwin adrianludwin left a comment

Choose a reason for hiding this comment

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

Basic code lgtm! We need to tweak the message, please work on this with Yiqi.

incubator/hnc/test/e2e/hns_test.go Outdated Show resolved Hide resolved
incubator/hnc/test/e2e/hns_test.go Outdated Show resolved Hide resolved
@GinnyJI GinnyJI force-pushed the force-flag branch 3 times, most recently from 0caaa84 to 71e6970 Compare October 5, 2020 20:33
fmt.Println("If you are sure you want to proceed with this operation, use the '--force' flag.")
fmt.Println("If you are not sure and would like to see what source objects would be overwritten," +
"please switch to 'Remove' first. Wait 10s before switching it to 'Propagate', which will" +
" list all the conflicts that you can resolve them by hand.")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add a link to https://github.com/kubernetes-sigs/multi-tenancy/blob/master/incubator/hnc/docs/user-guide/how-to.md#admin-types, and file a bug to update that topic to discuss this issue.

Also, I'm not sure that 10s is enough? In the doc you can go into more detail; maybe here I'd either say "a minute or two" or just say "to see how to enable propagation safely, see <link>."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Issue filed at #1172

@GinnyJI
Copy link
Contributor Author

GinnyJI commented Oct 5, 2020

/retest

Copy link
Contributor

@adrianludwin adrianludwin left a comment

Choose a reason for hiding this comment

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

lgtm after one last nit on the message, but I'll send this to Ryan now.

/lgtm
/approve
/hold
/assign @rjbez17

incubator/hnc/internal/kubectl/configset.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 5, 2020
If the user switch the propagation mode directly from 'Ignore' to
'Propagate', it will fail with an error message telling the user that a
'--force' flag is needed. '-f' is also supported as a shorthand.

Tested: make test-e2e
@k8s-ci-robot k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2020
@adrianludwin
Copy link
Contributor

/lgtm
/approve
/hold
/assign @rjbez17

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianludwin, GinnyJI

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

Copy link
Contributor

@yiqigao217 yiqigao217 left a comment

Choose a reason for hiding this comment

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

Sorry missed this PR earlier. BTW seems we still need e2e tests on switching modes between others, e.g. allow Remove => Propagate, etc.

@adrianludwin
Copy link
Contributor

Sorry missed this PR earlier. BTW seems we still need e2e tests on switching modes between others, e.g. allow Remove => Propagate, etc.

I don't think we need exhaustive testing, it doesn't have to be 100% black-box given how simple the feature is.

@rjbez17
Copy link

rjbez17 commented Oct 6, 2020

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 6, 2020
@k8s-ci-robot k8s-ci-robot merged commit b767c5f into kubernetes-retired:master Oct 6, 2020
@GinnyJI GinnyJI deleted the force-flag branch October 21, 2020 15:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants