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 dry-run flag to auth reconcile #64458

Merged
merged 1 commit into from
May 30, 2018
Merged

Add dry-run flag to auth reconcile #64458

merged 1 commit into from
May 30, 2018

Conversation

mrogers950
Copy link
Contributor

@mrogers950 mrogers950 commented May 29, 2018 •
edited
Loading 

The --dry-run flag has been enabled for kubectl auth reconcile

/assign @juanvallejo
cc @enj

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 29, 2018
@enj
Copy link
Member

enj commented May 29, 2018

/assign @deads2k

See if you agree with the flag names.

@mrogers950 probably need to update completions.

enj
enj reviewed May 29, 2018
View reviewed changes
pkg/kubectl/cmd/auth/reconcile.go Outdated
@@ -87,6 +90,9 @@ func NewCmdReconcile(f cmdutil.Factory, streams genericclioptions.IOStreams) *co
o.PrintFlags.AddFlags(cmd)

cmdutil.AddFilenameOptionFlags(cmd, o.FilenameOptions, "identifying the resource to reconcile.")
cmd.Flags().BoolVar(&o.DryRun, "dry-run", false, "If true, display results but do not submit changes")
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if printing needs to account for this somehow.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering if printing needs to account for this somehow.

I don't think that printers have dry run as a first level option. This the the domain of the command which is controlling behavior. The only printing side-effect is the name printer string which is already configurable.

@juanvallejo and @soltysh since we already litigated this once.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, we've agreed that printers don't have to do anything with dry-run, it's command specific.

@enj
Copy link
Member

enj commented May 29, 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 29, 2018
@mrogers950
Copy link
Contributor Author

@enj updated to account for a dry run printout

@mrogers950 mrogers950 force-pushed the reconcile-dryrun-additive branch 2 times, most recently from 60c3209 to dc342ef Compare May 29, 2018 19:39
@mrogers950 mrogers950 changed the title Add dry-run and additive-only flags to auth reconcile Add dry-run flag to auth reconcile May 29, 2018
@mrogers950
Copy link
Contributor Author

@enj @deads2k dropped additive-only from this for now

@enj
Copy link
Member

enj commented May 29, 2018

/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 29, 2018
deads2k
deads2k reviewed May 29, 2018
View reviewed changes
pkg/kubectl/cmd/auth/reconcile.go Outdated
@@ -87,6 +89,7 @@ func NewCmdReconcile(f cmdutil.Factory, streams genericclioptions.IOStreams) *co
o.PrintFlags.AddFlags(cmd)

cmdutil.AddFilenameOptionFlags(cmd, o.FilenameOptions, "identifying the resource to reconcile.")
cmd.Flags().BoolVar(&o.DryRun, "dry-run", false, "If true, display results but do not submit changes")
Copy link
Contributor

Choose a reason for hiding this comment

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

default value (third arg) should be o.DryRun

deads2k
deads2k reviewed May 29, 2018
View reviewed changes
pkg/kubectl/cmd/auth/reconcile.go Outdated
@@ -47,6 +47,8 @@ type ReconcileOptions struct {
PrintObject printers.ResourcePrinterFunc

genericclioptions.IOStreams

DryRun bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Move up with other flags that get bound (near the top).

@deads2k
Copy link
Contributor

deads2k commented May 29, 2018

Minor comments, but I'd like to see the the default value change

/approve
/hold

holding so the default value can change. @enj feel free to remove hold after update.

@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. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 29, 2018
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 29, 2018
@mrogers950
Copy link
Contributor Author

@deads2k updated with those changes

@enj
Copy link
Member

enj commented May 29, 2018

Comments addressed.

/hold cancel
/lgtm

@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 May 29, 2018
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 29, 2018
juanvallejo
juanvallejo reviewed May 29, 2018
View reviewed changes
pkg/kubectl/cmd/auth/reconcile.go Outdated
@@ -128,6 +130,9 @@ func (o *ReconcileOptions) Complete(cmd *cobra.Command, f cmdutil.Factory, args
return err
}

if o.DryRun {
o.PrintFlags = genericclioptions.NewPrintFlags("not reconciled (dry run)").WithTypeSetter(scheme.Scheme)
Copy link
Contributor

Choose a reason for hiding this comment

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

Normally, we set the print flags once, and if we need to account for a dry run message, we use the Complete method in the printflags struct: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/set/set_env.go#L219-L225

Add dry-run to auth reconcile
20cd94d 
Signed-off-by: Matt Rogers <mrogers@redhat.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 29, 2018
@enj
Copy link
Member

enj commented May 29, 2018

/lgtm

Retagging.

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

@juanvallejo thanks, updated

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@enj
Copy link
Member

enj commented May 29, 2018

/retest

1 similar comment
@mrogers950
Copy link
Contributor Author

/retest

soltysh
soltysh approved these changes May 30, 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 as well

@soltysh soltysh added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. sig/cli Categorizes an issue or PR as relevant to SIG CLI. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels May 30, 2018
@soltysh soltysh added this to the v1.11 milestone May 30, 2018
@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@deads2k @enj @mrogers950

Pull Request Labels
  • sig/cli: Pull Request will be escalated to these SIGs if needed.
  • priority/important-longterm: Escalate to the pull request owners; move out of the milestone after 1 attempt.
  • kind/cleanup: Adding tests, refactoring, fixing old bugs.
Help

@mrogers950 mrogers950 mentioned this pull request May 30, 2018
Closed
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 64322, 64210, 64458, 64232, 64370). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 75517f6 into kubernetes:master May 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juanvallejo juanvallejo juanvallejo left review comments

@enj enj enj left review comments

@deads2k deads2k deads2k left review comments

@soltysh soltysh soltysh approved these changes

Assignees

@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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.11
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@mrogers950 @enj @deads2k @k8s-ci-robot @k8s-github-robot @soltysh @juanvallejo