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

wire through name/success print flags #61496

Merged
merged 1 commit into from Mar 27, 2018

Conversation

@juanvallejo
Member

juanvallejo commented Mar 21, 2018

Release note:

NONE

Begin implementing pieces needed to retrieve name/success printers from a set of flags.
Proposal: https://docs.google.com/document/d/19ZZFVe9oD1KQmk5uExggRWtRl_hKGfYnBXvHZJlgEro/edit#heading=h.pnvbfi14v4zz

cc @soltysh @deads2k @pwittrock

// message indicating an action performed on a resource
type NameOrSuccessPrintFlags struct {
*NamePrintFlags
*SuccessPrintFlags

This comment has been minimized.

@juanvallejo

juanvallejo Mar 21, 2018

Member

@soltysh @deads2k maybe it would be better to compose NamePrintFlags in SuccessPrintFlags? (the name printer is used in the success printer regardless)

// ToPrinter returns a printer capable of printing a resource's "name"
// or success message based on the operation that took place on that object.
func (f *NameOrSuccessPrintFlags) ToPrinter(outputFormat string, operation string, dryRun bool) (ResourcePrinter, bool, error) {

This comment has been minimized.

@juanvallejo

juanvallejo Mar 21, 2018

Member

Unsure about the signature of this method.
Maybe a better approach would be to add "EnsurePrintWithOperation....", or something along those lines?
At least that way, we could keep a consistent ToPrinter method signature across all PrintFlags.

@soltysh soltysh self-assigned this Mar 22, 2018

@soltysh

This comment has been minimized.

Contributor

soltysh commented Mar 22, 2018

/ok-to-test

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 22, 2018

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

@juanvallejo @soltysh

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/feature: New functionality.
Help

@soltysh soltysh referenced this pull request Mar 22, 2018

Closed

Kubectl refactorings umbrella issue #61471

4 of 5 tasks complete
@soltysh

I was imagining both of these printers squashed into one. And the user (being the developer) would pass a flag whether he wants one or the other at construction. The underlying implementation is one, why we'd need to structs in that case.

"k8s.io/apimachinery/pkg/runtime"
"k8s.io/kubernetes/pkg/kubectl/scheme"

This comment has been minimized.

@soltysh

soltysh Mar 22, 2018

Contributor

Unnecessary blank line.

"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
)
// NamePrintFlags provides default flags necessary for json/yaml printing.

This comment has been minimized.

@soltysh

soltysh Mar 22, 2018

Contributor

name printing not json/yaml

// Returns false if the specified outputFormat does not match a supported format.
// Supported format types can be found in pkg/printers/printers.go
func (f *NamePrintFlags) ToPrinter(outputFormat string) (ResourcePrinter, bool, error) {
if len(outputFormat) == 0 {

This comment has been minimized.

@soltysh

soltysh Mar 22, 2018

Contributor

Again turn this into nice switch.

return namePrinter, true, nil
}
return &SuccessPrinter{NamePrinter: namePrinter, DryRun: f.DryRun, Operation: f.Operation}, true, nil

This comment has been minimized.

@juanvallejo

juanvallejo Mar 22, 2018

Member

@soltysh instead of editing the NamePrinter's PrintObject method directly to handle "success" printing, I left the NamePrinter wrapped in a printer that can handle the "success" case specifically instead. PTAL

@BenTheElder

This comment has been minimized.

Member

BenTheElder commented Mar 22, 2018

/retest

@BenTheElder

This comment has been minimized.

Member

BenTheElder commented Mar 22, 2018

[network flake]
/retest

@juanvallejo

This comment has been minimized.

Member

juanvallejo commented Mar 23, 2018

/test pull-kubernetes-e2e-gce

1 similar comment
@juanvallejo

This comment has been minimized.

Member

juanvallejo commented Mar 23, 2018

/test pull-kubernetes-e2e-gce

@soltysh

/lgtm
/approve no-issue

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 25, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Mar 25, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juanvallejo, 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 removed the lgtm label Mar 26, 2018

@juanvallejo

This comment has been minimized.

Member

juanvallejo commented Mar 26, 2018

@soltysh had to rebase, mind re-tagging once more?

@soltysh

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 26, 2018

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 27, 2018

Automatic merge from submit-queue (batch tested with PRs 61546, 61038, 61575, 60779, 61496). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 7d4bf1c into kubernetes:master Mar 27, 2018

14 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation juanvallejo authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@juanvallejo juanvallejo deleted the juanvallejo:jvallejo/add-success-printer-flags branch Mar 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment