add EditOptions to make edit reusable #39404

adohe-zz · 2017-01-04T04:05:32Z

FYI. Added EditOptions to decouple edit implementation from cobra command, thus make edit reusable. Once this get merged, we can refactor create commands to put them in their own package. @kubernetes/sig-cli-misc please give a quick review, and I would like to get this merged asap.

k8s-reviewable · 2017-01-04T04:05:40Z

This change is

fabianofranz · 2017-01-10T18:32:45Z

@adohe is this no longer WIP?

pwittrock · 2017-01-11T16:28:30Z

Also, are the test failures legit?

0xmichalis · 2017-01-11T16:36:57Z

@adohe thanks for working on this! Related issue: #38761

adohe-zz · 2017-01-15T15:16:10Z

@fabianofranz @pwittrock ready for review, ptal.

fabianofranz · 2017-01-16T18:21:50Z

@k8s-bot verify test this

pwittrock · 2017-01-17T22:01:44Z

@k8s-bot verify test this

pwittrock · 2017-01-17T22:11:09Z

pkg/kubectl/cmd/util/helpers.go

@@ -408,13 +414,22 @@ func AddApplyAnnotationFlags(cmd *cobra.Command) {
 	cmd.Flags().Bool(ApplyAnnotationsFlag, false, "If true, the configuration of current object will be saved in its annotation. This is useful when you want to perform kubectl apply on this object in the future.")
 }

+func AddApplyAnnotationVarFlags(cmd *cobra.Command, applyAnnotation *bool) {


WDYT of creating "Get" versions of these flags as well so they can be used in your code above?

0xmichalis · 2017-01-18T00:55:37Z

pkg/kubectl/cmd/util/editor/editoptions.go

+		containsError := false
+		var infos []*resource.Info
+		for {
+			switch o.EditMode {


Can't this check be moved out of the loop?

shiywang · 2017-02-14T07:17:23Z

pkg/kubectl/resource/visitor.go

@@ -648,6 +648,16 @@ func RetrieveLazy(info *Info, err error) error {
 	return nil
 }

+// CreateAndRefresh creates an object from input info and refreshes info with that object
+func CreateAndRefresh(info *Info) error {


Is this function original move from pkg/kubectl/cmd/create.go and made it public ? if so, can we delete the previous one and only keep this one ?

adohe-zz · 2017-04-09T11:00:19Z

Rebased and test cases fixed. @pwittrock @fabianofranz please take a look, I would like to get this merged asap.

adohe-zz · 2017-04-09T11:00:28Z

/assign @pwittrock

0xmichalis · 2017-04-09T12:56:42Z

pkg/kubectl/cmd/util/editor/editoptions.go

+	"k8s.io/kubernetes/pkg/printers"
+	"k8s.io/kubernetes/pkg/util/crlf"
+
+	"github.com/evanphx/json-patch"


Move these imports in between stdlib and kubernetes imports

comment addressed.

0xmichalis · 2017-04-09T12:58:01Z

pkg/kubectl/cmd/util/editor/editoptions.go

+// Complete completes all the required options
+func (o *EditOptions) Complete(f cmdutil.Factory, out, errOut io.Writer, args []string) error {
+	if o.EditMode != NormalEditMode && o.EditMode != EditBeforeCreateMode {
+		return fmt.Errorf("Unsupported edit mode %q", o.EditMode)


errors in fmt.Errorf / errors.New should start lowercased

0xmichalis · 2017-04-09T12:58:50Z

pkg/kubectl/cmd/util/editor/editoptions.go

+		return err
+	}
+	b := resource.NewBuilder(mapper, f.CategoryExpander(), typer, resource.ClientMapperFunc(f.UnstructuredClientForMapping), unstructured.UnstructuredJSONScheme)
+	if o.EditMode == NormalEditMode {


Can you add a comment here why is this needed for normal edit mode?

0xmichalis · 2017-04-09T13:02:47Z

pkg/kubectl/cmd/util/editor/editoptions.go

+			case EditBeforeCreateMode:
+				err = visitToCreate(updatedVisitor, o.Mapper, o.Out)
+			default:
+				err = fmt.Errorf("Unsupported edit mode %q", o.EditMode)


shiywang · 2017-04-10T03:09:45Z

pkg/kubectl/cmd/util/editor/editoptions.go, line 349 at r4 (raw file):

	default:
		// if format is not specified, use yaml as default
		return &editPrinterOptions{

I'm not sure if this is right, previously if specify a invalid ouput, it will prompt "The flag 'output' must be one of yaml|json" should we keep error output compatible ?

Comments from Reviewable

shiywang · 2017-04-10T03:34:37Z

just have a quick look, generally lgtm, thanks for this work, I will add a third mode ApplyEditMode when this pr get merged : )
cc @ymqytw

mengqiy · 2017-04-10T16:46:51Z

pkg/kubectl/cmd/edit.go

 	"k8s.io/kubernetes/pkg/util/i18n"

-	jsonpatch "github.com/evanphx/json-patch"
-	"github.com/golang/glog"
 	"github.com/spf13/cobra"


We should move this up per #39404 (comment)

shiywang · 2017-04-17T08:02:31Z

@adohe is there any progress on this ? if you really busy, I would like to pick up the rest of work

adohe-zz · 2017-04-18T12:45:44Z

rebased and comments addressed, ptal.

mengqiy · 2017-04-19T04:10:39Z

Review status: 0 of 9 files reviewed at latest revision, 10 unresolved discussions.

pkg/kubectl/cmd/util/helpers.go, line 430 at r5 (raw file):

func AddApplyAnnotationVarFlags(cmd *cobra.Command, applyAnnotation *bool) {
	cmd.Flags().BoolVar(applyAnnotation, ApplyAnnotationsFlag, false, "If true, the configuration of current object will be saved in its annotation. This is useful when you want to perform kubectl apply on this object in the future.")

Flag help text inconsistent.

pkg/kubectl/cmd/util/editor/editoptions.go, line 349 at r4 (raw file):

Previously, shiywang (Shiyang Wang) wrote…

I'm not sure if this is right, previously if specify a invalid ouput, it will prompt "The flag 'output' must be one of yaml|json" should we keep error output compatible ?

This is not identical to before.

Before is
switch format {
case "json":
// json printer
case "yaml", "": // specify yaml or unspecified
// yaml printer
default:
// error
}

Comments from Reviewable

mengqiy · 2017-04-21T17:32:48Z

I'm OK to merge this as is and let @shiywang to address the comments left in a followup PR.
This PR is blocking @shiywang's PR #42256.
It seems @adohe don't have bandwidth on this.

0xmichalis · 2017-04-23T16:03:43Z

sgtm

mengqiy · 2017-04-24T19:06:31Z

/lgtm
And @shiywang will address the rest.

k8s-github-robot · 2017-04-24T19:07:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adohe, mengqiy

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

~~pkg/kubectl/OWNERS~~ [adohe]

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

k8s-github-robot · 2017-04-24T23:20:39Z

Automatic merge from submit-queue

mengqiy · 2017-04-25T06:37:56Z

Created #44905 to make sure the comments left will be addressed.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 4, 2017

k8s-github-robot assigned pwittrock Jan 4, 2017

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Jan 4, 2017

0xmichalis added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jan 11, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 11, 2017

adohe-zz force-pushed the refactor_edit branch from bf0faa5 to fb38b1a Compare January 15, 2017 15:15

adohe-zz changed the title ~~[WIP] add EditOptions to make edit reusable~~ add EditOptions to make edit reusable Jan 15, 2017

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 15, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 16, 2017

pwittrock reviewed Jan 17, 2017

View reviewed changes

0xmichalis reviewed Jan 18, 2017

View reviewed changes

k8s-github-robot assigned fabianofranz and unassigned pwittrock Jan 30, 2017

apelisse assigned pwittrock Jan 31, 2017

k8s-github-robot assigned brendandburns Jan 31, 2017

adohe-zz force-pushed the refactor_edit branch from fb38b1a to 8b4233f Compare February 11, 2017 13:03

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 11, 2017

shiywang reviewed Feb 14, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2017

pwittrock removed their assignment Apr 5, 2017

adohe-zz force-pushed the refactor_edit branch from 810280a to 7fc7847 Compare April 9, 2017 09:55

k8s-ci-robot assigned pwittrock Apr 9, 2017

0xmichalis reviewed Apr 9, 2017

View reviewed changes

pwittrock assigned mengqiy and unassigned pwittrock Apr 9, 2017

mengqiy reviewed Apr 10, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 13, 2017

refactor edit to remove cobra dependency and also make it reusable

5ed1ab6

adohe-zz force-pushed the refactor_edit branch from 7fc7847 to 5ed1ab6 Compare April 18, 2017 12:45

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 18, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 24, 2017

k8s-github-robot merged commit ed539fb into kubernetes:master Apr 24, 2017

mengqiy mentioned this pull request Apr 25, 2017

Clean up edit and refactor edit #44905

Closed

adohe-zz deleted the refactor_edit branch June 8, 2018 11:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add EditOptions to make edit reusable #39404

add EditOptions to make edit reusable #39404

adohe-zz commented Jan 4, 2017

k8s-reviewable commented Jan 4, 2017

fabianofranz commented Jan 10, 2017

pwittrock commented Jan 11, 2017

0xmichalis commented Jan 11, 2017

adohe-zz commented Jan 15, 2017

fabianofranz commented Jan 16, 2017

pwittrock commented Jan 17, 2017

pwittrock Jan 17, 2017

0xmichalis Jan 18, 2017

shiywang Feb 14, 2017 •

edited

adohe-zz commented Apr 9, 2017

adohe-zz commented Apr 9, 2017

0xmichalis Apr 9, 2017

adohe-zz Apr 18, 2017

0xmichalis Apr 9, 2017

0xmichalis Apr 9, 2017

adohe-zz Apr 18, 2017

0xmichalis Apr 9, 2017

adohe-zz Apr 18, 2017

shiywang commented Apr 10, 2017

shiywang commented Apr 10, 2017 •

edited

mengqiy Apr 10, 2017

shiywang commented Apr 17, 2017 •

edited

adohe-zz commented Apr 18, 2017

mengqiy commented Apr 19, 2017

mengqiy commented Apr 21, 2017

0xmichalis commented Apr 23, 2017

mengqiy commented Apr 24, 2017

k8s-github-robot commented Apr 24, 2017

k8s-github-robot commented Apr 24, 2017

mengqiy commented Apr 25, 2017

add EditOptions to make edit reusable #39404

add EditOptions to make edit reusable #39404

Conversation

adohe-zz commented Jan 4, 2017

k8s-reviewable commented Jan 4, 2017

fabianofranz commented Jan 10, 2017

pwittrock commented Jan 11, 2017

0xmichalis commented Jan 11, 2017

adohe-zz commented Jan 15, 2017

fabianofranz commented Jan 16, 2017

pwittrock commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiywang Feb 14, 2017 • edited

Choose a reason for hiding this comment

adohe-zz commented Apr 9, 2017

adohe-zz commented Apr 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiywang commented Apr 10, 2017

shiywang commented Apr 10, 2017 • edited

Choose a reason for hiding this comment

shiywang commented Apr 17, 2017 • edited

adohe-zz commented Apr 18, 2017

mengqiy commented Apr 19, 2017

mengqiy commented Apr 21, 2017

0xmichalis commented Apr 23, 2017

mengqiy commented Apr 24, 2017

k8s-github-robot commented Apr 24, 2017

k8s-github-robot commented Apr 24, 2017

mengqiy commented Apr 25, 2017

shiywang Feb 14, 2017 •

edited

shiywang commented Apr 10, 2017 •

edited

shiywang commented Apr 17, 2017 •

edited