oc policy can-i --list output is not parser friendly #15125

abstractj · 2017-07-10T22:39:09Z

@fabianofranz this fixes #11147 is just a rebase from the changes that @juanvallejo made.
He pretty much did all the hard work on it.

fixes #11147

abstractj · 2017-07-11T13:30:05Z

@juanvallejo interesting. Jenkins is misbehaving, but looking at the console it does not seems related with your change :/

fabianofranz · 2017-07-11T19:07:48Z

pkg/cmd/admin/policy/cani.go

@@ -79,6 +83,7 @@ func NewCmdCanI(name, fullName string, f *clientcmd.Factory, out io.Writer) *cob
 	cmd.Flags().StringVar(&o.User, "user", o.User, "Check the specified action using this user instead of your user.")
 	cmd.Flags().StringSliceVar(&o.Groups, "groups", o.Groups, "Check the specified action using these groups instead of your groups.")

+	kcmdutil.AddPrinterFlags(cmd)


This adds a number of additional flags (--sort-by, --no-headers, etc). Do they all behave correctly?

@fabianofranz from what I tested manually, it does not crash, but the same time it takes no effect. I'm not sure if we should worry about it.

from what I tested manually, it does not crash, but the same time it takes no effect. I'm not sure if we should worry about it.

We would get this for free if we switch to using the HumanReadablePrinter :)

If you are both in agreement on it, why not!? Let's switch to this.

fabianofranz · 2017-07-11T19:13:41Z

pkg/cmd/admin/policy/cani.go

+	}
+
+	if o.Printer != nil {
+		return o.Printer.PrintObj(rulesReviewResult, o.Out)
 	}

 	writer := tabwriter.NewWriter(o.Out, tabwriterMinWidth, tabwriterWidth, tabwriterPadding, tabwriterPadChar, tabwriterFlags)


Would it make sense to get rid of this+4 lines and always rely on the printer, with the HumanReadablePrinter by default?

@fabianofranz not sure TBH. Maybe @juanvallejo can tell us more. One thing that I saw at the docs is the fact that HumanReadablePrinter is not threadsafe. Should we be concerned about it?

What would be the practical benefit of using it? Get rid of AddPrinterFlags ?

One thing that I saw at the docs is the fact that HumanReadablePrinter is not threadsafe. Should we be concerned about it?

No, not for this use-case in my opinion

What would be the practical benefit of using it? Get rid of AddPrinterFlags ?

We would get functionality for additional printer flags (no-headers, show-all, wide, ...)

Would it make sense to get rid of this+4 lines and always rely on the printer, with the HumanReadablePrinter by default?

I agree makes sense, though I think we might have to add a handler here with the current printing behavior in DescribePolicyRule

@juanvallejo could you elaborate more? Looking at the code if we add that, that might affect DescribeRoleBinding too. Not sure if we want that.

could you elaborate more?

Sure, I was suggesting that we should add a new handler func (kind of like was done for buildConfigs, for example) that knows how to print a list of policy rules while taking human-readable printer options into account. We could re-use some of the code that now exists in DescribePolicyRule.

@juanvallejo is this somewhat related with what you mean https://github.com/abstractj/origin/commit/b69cc8d49b8b5076eea0ed269417362143ff16a8 ?

Yeah, that looks like a good start

@juanvallejo just updated the code https://github.com/abstractj/origin/commit/b6a235530ca969cb3a03e32f95a307c7c6a4abbb. But I feel like there's something missing, because it's not working :)

Some considerations:

If we remove kcmdutil.AddPrinterFlags we lose --output. I believe that if that's the idea, might be necessary to add something like:

cmd.Flags().StringVar(&o.Output, "output", o.Output, "YAML or JSON") output := kcmdutil.GetFlagString(cmd, "output") if len(output) > 0 { //print }

The code duplicates the same code at DescribePolicyRule, should we remove this function? I guess not, but worth to check.

@fabianofranz fyi we've been discussing the changes here https://github.com/abstractj/origin/commit/b6a235530ca969cb3a03e32f95a307c7c6a4abbb#diff-06058d45973b9854e2a088207ab6ee7fR383. I haven't included it to this PR, because my changes atm will revert to the original state.

fabianofranz · 2017-07-11T19:18:56Z

I'd also suggest adding a couple tests with --list --output=<foo> to test/cmd/authentication.sh, @abstractj there's a section for can-i tests there already, should be pretty straightforward. ;)

abstractj · 2017-07-12T13:22:48Z

@fabianofranz sure, will look about how to add tests for it.

abstractj · 2017-07-21T12:04:35Z

@juanvallejo @fabianofranz PR updated with tests. Not sure if they are consistent enough for this change, but please let me know if there's anything else that should be updated.

juanvallejo · 2017-07-21T14:37:18Z

pkg/cmd/admin/policy/cani.go

@@ -132,14 +130,15 @@ func (o *canIOptions) Complete(cmd *cobra.Command, f *clientcmd.Factory, args []

 	output := kcmdutil.GetFlagString(cmd, "output")
 	if len(output) > 0 {


Did you mean to keep this conditional block here?

juanvallejo · 2017-07-21T14:38:14Z

pkg/cmd/admin/policy/cani.go

 	}

+	printer, err := f.PrinterForCommand(cmd, false, nil, printers.PrintOptions{})


you should handle this error

juanvallejo · 2017-07-21T15:09:17Z

pkg/cmd/admin/policy/cani.go

+
+	}
+
+	printer, err := f.PrinterForCommand(cmd, false, nil, printers.PrintOptions{})


@fabianofranz I have not yet tested flags like --sort-by with this PR;
@abstractj maybe we should handle options like no-headers here:

printers.PrintOptions{ NoHeaders: kcmdutil.GetFlagBool(cmd, "no-headers"), }

Also, -o name renders the output: <unknown>/<unknown>, although not sure if that might be related to this pr

openshift-bot · 2017-07-21T17:31:58Z

Evaluated for origin test up to 554b0ff

abstractj · 2017-07-21T17:35:43Z

@juanvallejo just updated the PR based on your comments. Regards to what you said:

@abstractj maybe we should handle options like no-headers here:

printers.PrintOptions{
    NoHeaders: kcmdutil.GetFlagBool(cmd, "no-headers"),
}
Also, -o name renders the output: <unknown>/<unknown>, although not sure if that might be related to this pr

I don't bother to do more changes related to this. I'm just afraid of we never reach an end, because you know, there's always more to improve. Anyways, whatever you guys decide is fine for me.

If you choose to open an enhancement, feel free to assign to me. Otherwise, let's iterate over this PR.

juanvallejo · 2017-07-21T18:01:03Z

I don't bother to do more changes related to this. I'm just afraid of we never reach an end, because you know, there's always more to improve. Anyways, whatever you guys decide is fine for me.

I'd be okay with just changing AddPrinterFlags to kcmdutil.AddOutputFlags and maybe in a later revision considering adding support for additional flags; though will defer the final say on this to @fabianofranz

openshift-bot · 2017-07-21T19:17:58Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/3407/) (Base Commit: e97ba37) (PR Branch Commit: 554b0ff)

abstractj · 2017-07-27T14:09:22Z

/retest

juanvallejo · 2017-07-27T14:35:59Z

you're not using this package anywhere in cani.go

abstractj · 2017-07-27T19:28:00Z

/retest

abstractj · 2017-07-28T00:58:04Z

@juanvallejo green again!

Inclusion of printer handlers for oc policy. The goal is to permit people to print the output in YAML or JSON by running: oc policy can-i --list --output=yaml Signed-off-by: Bruno Oliveira <[email protected]>

pweil- · 2017-07-31T16:33:02Z

@abstractj @fabianofranz @juanvallejo ok, let's go with AddOutputFlags for now to get what we need for the issue and a follow up issue to track adding other options is fine with me.

fabianofranz · 2017-08-01T23:03:21Z

/lgtm
/approve

openshift-merge-robot · 2017-08-01T23:03:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abstractj, fabianofranz

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~contrib/completions/OWNERS~~ [fabianofranz]
~~pkg/oc/OWNERS~~ [fabianofranz]
~~test/cmd/OWNERS~~ [fabianofranz]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

enj · 2017-08-02T03:06:37Z

/test end_to_end

openshift-merge-robot · 2017-08-02T13:19:44Z

Automatic merge from submit-queue (batch tested with PRs 15512, 15582, 15125, 15590, 15593)

fabianofranz reviewed Jul 11, 2017

View reviewed changes

abstractj requested review from fabianofranz and juanvallejo July 21, 2017 12:05

juanvallejo reviewed Jul 21, 2017

View reviewed changes

abstractj requested a review from juanvallejo July 21, 2017 17:32

openshift-merge-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 24, 2017

openshift-merge-robot assigned 0xmichalis and mfojtik Jul 25, 2017

openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 27, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2017

openshift-merge-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2017

0xmichalis assigned juanvallejo and unassigned mfojtik and 0xmichalis Jul 29, 2017

juanvallejo and others added 2 commits July 31, 2017 09:21

oc policy can-i --list output is not parser friendly

761f6cf

Add print handlers

3c8706c

Inclusion of printer handlers for oc policy. The goal is to permit people to print the output in YAML or JSON by running: oc policy can-i --list --output=yaml Signed-off-by: Bruno Oliveira <[email protected]>

openshift-ci-robot assigned fabianofranz Aug 1, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 1, 2017

juanvallejo approved these changes Aug 1, 2017

View reviewed changes

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2017

openshift-merge-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2017

openshift-merge-robot merged commit 68da586 into openshift:master Aug 2, 2017

		@@ -132,14 +130,15 @@ func (o canIOptions) Complete(cmd cobra.Command, f *clientcmd.Factory, args []

		output := kcmdutil.GetFlagString(cmd, "output")
		if len(output) > 0 {

		}

		printer, err := f.PrinterForCommand(cmd, false, nil, printers.PrintOptions{})

oc policy can-i --list output is not parser friendly #15125

oc policy can-i --list output is not parser friendly #15125

Uh oh!

Conversation

abstractj commented Jul 10, 2017 • edited by fabianofranz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abstractj commented Jul 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juanvallejo Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abstractj Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabianofranz commented Jul 11, 2017

Uh oh!

abstractj commented Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abstractj commented Jul 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-bot commented Jul 21, 2017

Uh oh!

abstractj commented Jul 21, 2017

Uh oh!

juanvallejo commented Jul 21, 2017

Uh oh!

openshift-bot commented Jul 21, 2017

Uh oh!

abstractj commented Jul 27, 2017

Uh oh!

juanvallejo commented Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abstractj commented Jul 27, 2017

Uh oh!

abstractj commented Jul 28, 2017

Uh oh!

pweil- commented Jul 31, 2017

Uh oh!

fabianofranz commented Aug 1, 2017

Uh oh!

openshift-merge-robot commented Aug 1, 2017

Uh oh!

enj commented Aug 2, 2017

Uh oh!

openshift-merge-robot commented Aug 2, 2017

abstractj commented Jul 10, 2017 •

edited by fabianofranz

Loading

juanvallejo Jul 12, 2017 •

edited

Loading

abstractj Jul 18, 2017 •

edited

Loading

abstractj commented Jul 12, 2017 •

edited

Loading

juanvallejo commented Jul 27, 2017 •

edited

Loading