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

Add more kubectl auth reconcile flags #64541

Merged
merged 1 commit into from
Jun 20, 2018
Merged

Add more kubectl auth reconcile flags #64541

merged 1 commit into from
Jun 20, 2018
+11 −7

Conversation

mrogers950
Copy link
Contributor

@mrogers950 mrogers950 commented May 31, 2018 •
edited
Loading 

The --remove-extra-subjects and --remove-extra-permissions flags have been enabled for kubectl auth reconcile

/assign @deads2k
cc @enj @juanvallejo

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 31, 2018
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 31, 2018
@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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 31, 2018
enj
enj reviewed May 31, 2018
View reviewed changes
pkg/kubectl/cmd/auth/reconcile.go Outdated
@@ -89,6 +90,7 @@ func NewCmdReconcile(f cmdutil.Factory, streams genericclioptions.IOStreams) *co

cmdutil.AddFilenameOptionFlags(cmd, o.FilenameOptions, "identifying the resource to reconcile.")
cmd.Flags().BoolVar(&o.DryRun, "dry-run", o.DryRun, "If true, display results but do not submit changes")
cmd.Flags().BoolVar(&o.AdditiveOnly, "additive-only", true, "If true, preserve modified roles and bindings")
Copy link
Member

Choose a reason for hiding this comment

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

I assume the true should be set in o and then passed as the third arg?

@enj
Copy link
Member

enj commented May 31, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 31, 2018
@mrogers950
Copy link
Contributor Author

Updated.

deads2k
deads2k reviewed May 31, 2018
View reviewed changes
pkg/kubectl/cmd/auth/reconcile.go Outdated
@@ -89,6 +91,7 @@ func NewCmdReconcile(f cmdutil.Factory, streams genericclioptions.IOStreams) *co

cmdutil.AddFilenameOptionFlags(cmd, o.FilenameOptions, "identifying the resource to reconcile.")
cmd.Flags().BoolVar(&o.DryRun, "dry-run", o.DryRun, "If true, display results but do not submit changes")
cmd.Flags().BoolVar(&o.AdditiveOnly, "additive-only", o.AdditiveOnly, "If true, preserve modified roles and bindings")
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer using CLI commands that bool flags default to false.

@kubernetes/sig-cli-maintainers @soltysh

@deads2k
Copy link
Contributor

deads2k commented May 31, 2018

This needs to wait until 1.12

@enj
Copy link
Member

enj commented May 31, 2018

This needs to wait until 1.12

SGTM

I prefer using CLI commands that bool flags default to false.

--tighten?

@mrogers950
Copy link
Contributor Author

--tighten?

how about --restore ?

@liggitt
Copy link
Member

liggitt commented May 31, 2018

I think separate control over removing extra permissions and removing extra subjects makes sense, and makes for very clear flags

--remove-extra-permissions
--remove-extra-subjects

Add more kubectl auth reconcile flags
9f3b975 
Adds --remove-extra-permissions and --remove-extra-subjects

Signed-off-by: Matt Rogers <[email protected]>
@mrogers950 mrogers950 changed the title Add additive-only flag to auth reconcile Add more kubectl auth reconcile flags May 31, 2018
@mrogers950
Copy link
Contributor Author

Updated with separated flags as @liggitt suggested.

@mrogers950
Copy link
Contributor Author

/retest

@enj
Copy link
Member

enj commented Jun 1, 2018

/lgtm
/retest

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2018
@mrogers950
Copy link
Contributor Author

/retest

soltysh
soltysh approved these changes Jun 5, 2018
View reviewed changes
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enj, mrogers950, soltysh

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 5, 2018
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

4 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 63580, 63744, 64541, 64502, 64100). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 5e31112 into kubernetes:master Jun 20, 2018
@mrogers950 mrogers950 mentioned this pull request Jul 11, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k deads2k left review comments

@enj enj enj left review comments

@soltysh soltysh soltysh approved these changes

@eparis eparis Awaiting requested review from eparis

@ghodss ghodss Awaiting requested review from ghodss

Assignees

@soltysh soltysh

@enj enj

@deads2k deads2k

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. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@mrogers950 @enj @deads2k @liggitt @k8s-ci-robot @fejta-bot @k8s-github-robot @soltysh