Change the order of Validate() and Complete() for oc set volume #19169

nak3 · 2018-03-31T05:29:55Z

When oc set volum was run, cli proceeds
Validate()->Complete(). This order is different from any other
cli implementations. Currently there are no isseus, but this may
effect future implementation so fix it now.

mfojtik · 2018-04-03T11:17:14Z

/sig master
/assign maszulik

openshift-ci-robot · 2018-04-03T11:17:15Z

@mfojtik: GitHub didn't allow me to assign the following users: maszulik.

Note that only openshift members and repo collaborators can be assigned.

In response to this:

/sig master
/assign maszulik

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

soltysh · 2018-04-04T10:49:31Z

pkg/oc/cli/cmd/set/volume.go

@@ -411,9 +415,6 @@ func (v *VolumeOptions) Complete(f *clientcmd.Factory, cmd *cobra.Command, out,
 		}
 		v.AddOpts.ClaimSize = q.String()
 	}
-	if len(v.AddOpts.DefaultMode) == 0 {


That's actually good piece of code that should be left here. Removing it from here you're creating two places where you're setting the default mode.

@soltysh Thank you. But if DefaultMode was set here, other storage type (emptydir,hostpath ..) will be broken with current validation check. You can refer to the eror
Is it OK to change them? I think codes under func (a *AddVolumeOptions) Validate(isAddOp bool) need a big change.

Feel free to fix it as necessary, but ensure you don't create duplicate code paths.

soltysh · 2018-04-04T10:49:54Z

pkg/oc/cli/cmd/set/volume.go

@@ -307,7 +307,9 @@ func (a *AddVolumeOptions) Validate(isAddOp bool) error {
 				if len(a.SecretName) == 0 {
 					return errors.New("must provide --secret-name for --type=secret")
 				}
-				if len(a.DefaultMode) > 0 {
+				if len(a.DefaultMode) == 0 {


Revert previous version.

soltysh · 2018-04-04T10:49:59Z

pkg/oc/cli/cmd/set/volume.go

@@ -316,7 +318,9 @@ func (a *AddVolumeOptions) Validate(isAddOp bool) error {
 				if len(a.ConfigMapName) == 0 {
 					return errors.New("must provide --configmap-name for --type=configmap")
 				}
-				if len(a.DefaultMode) > 0 {
+				if len(a.DefaultMode) == 0 {


nak3 · 2018-04-07T04:34:14Z

/retest

nak3 · 2018-04-07T05:29:06Z

@soltysh Thank you. Updated.

juanvallejo · 2018-04-09T18:54:28Z

pkg/oc/cli/cmd/set/volume.go

+	case numOps == 0:
+		v.List = true
+	case numOps > 1:
+		return errors.New("you may only specify one operation at a time")


This looks like a validation error; keeping it in Validate seems right to me

hm, since numOps is used to set the value of v.List to true, I'd be okay with still moving this to Validate and having that field completed there - still better than having all of the other completion logic that was in that method before. @soltysh for an opinion

Let's leave this as is. I don't think we need to enforce every check be only in Validate.

juanvallejo · 2018-04-09T19:06:55Z

pkg/oc/cli/cmd/set/volume.go

 	// In case of volume source ignore the default volume type
-	if len(v.AddOpts.Source) > 0 {
-		v.AddOpts.Type = ""
+	if len(a.Source) > 0 {


since Validate used to be called first, and this block was called in Complete, move this block to the end of this method

juanvallejo · 2018-04-09T19:08:00Z

pkg/oc/cli/cmd/set/volume.go

-		v.AddOpts.CreateClaim = true
-		if len(v.AddOpts.ClaimName) == 0 {
-			v.AddOpts.ClaimName = names.SimpleNameGenerator.GenerateName("pvc-")
+	if len(a.ClaimSize) > 0 {


same with this block

juanvallejo · 2018-04-09T19:23:06Z

pkg/oc/cli/cmd/set/volume.go

 	case "":
 	default:
 		return errors.New("--claim-mode must be one of ReadWriteOnce (rwo), ReadWriteMany (rwm), or ReadOnlyMany (rom)")
 	}
+
+	if len(a.Type) == 0 {
+		if len(a.ClaimName) > 0 || len(a.ClaimSize) > 0 {


Please turn this into a switch, should make it a bit easier to read:

if len(a.Type) == 0 { switch { case len(a.ClaimName) > 0 || len(a.ClaimSize) > 0: a.Type = "persistentvolumeclaim" a.TypeChanged = true case len(a.SecretName) > 0: a.Type = "secret" a.TypeChanged = true case len(a.ConfigMapName) > 0: a.Type = "configmap" a.TypeChanged = true case len(a.Path) > 0: a.Type = "hostpath" a.TypeChanged = true default: a.Type = "emptydir" } }

juanvallejo · 2018-04-09T19:40:34Z

Overall, the changes look good.
A few comments regarding the calling order of a few things.

Please squash commits

nak3 · 2018-04-09T22:50:48Z

/test cmd

nak3 · 2018-04-09T23:41:34Z

/test cmd

mmm... cmd test failed, but it should not related to this change.

nak3 · 2018-04-10T00:30:43Z

/test cmd

nak3 · 2018-04-10T01:26:12Z

@juanvallejo @soltysh Thank you! Updated & Squashed.

juanvallejo · 2018-04-10T18:31:02Z

/test gcp

soltysh

/lgtm
/approve

openshift-ci-robot · 2018-04-12T10:37:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nak3, 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:

~~pkg/oc/OWNERS~~ [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

soltysh · 2018-04-12T10:37:26Z

/retest

openshift-ci-robot requested review from enj and soltysh March 31, 2018 05:29

openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 31, 2018

openshift-ci-robot added the sig/master label Apr 3, 2018

soltysh self-assigned this Apr 3, 2018

soltysh suggested changes Apr 4, 2018

View reviewed changes

juanvallejo reviewed Apr 9, 2018

View reviewed changes

Refactor validate and complete in oc set volume

ebd7209

soltysh approved these changes Apr 12, 2018

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 12, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 12, 2018

openshift-merge-robot merged commit f83dfd7 into openshift:master Apr 12, 2018

nak3 deleted the set-volume branch April 13, 2018 12:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change the order of Validate() and Complete() for oc set volume #19169

Change the order of Validate() and Complete() for oc set volume #19169

nak3 commented Mar 31, 2018

mfojtik commented Apr 3, 2018

openshift-ci-robot commented Apr 3, 2018

soltysh Apr 4, 2018

nak3 Apr 4, 2018

soltysh Apr 5, 2018

soltysh Apr 4, 2018

soltysh Apr 4, 2018

nak3 commented Apr 7, 2018

nak3 commented Apr 7, 2018

juanvallejo Apr 9, 2018

juanvallejo Apr 9, 2018

soltysh Apr 9, 2018

juanvallejo Apr 9, 2018

juanvallejo Apr 9, 2018

juanvallejo Apr 9, 2018

juanvallejo commented Apr 9, 2018

nak3 commented Apr 9, 2018

nak3 commented Apr 9, 2018 •

edited

Loading

nak3 commented Apr 10, 2018

nak3 commented Apr 10, 2018 •

edited

Loading

juanvallejo commented Apr 10, 2018

soltysh left a comment

openshift-ci-robot commented Apr 12, 2018

soltysh commented Apr 12, 2018

Change the order of Validate() and Complete() for oc set volume #19169

Change the order of Validate() and Complete() for oc set volume #19169

Conversation

nak3 commented Mar 31, 2018

mfojtik commented Apr 3, 2018

openshift-ci-robot commented Apr 3, 2018

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

nak3 commented Apr 7, 2018

nak3 commented Apr 7, 2018

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

juanvallejo commented Apr 9, 2018

nak3 commented Apr 9, 2018

nak3 commented Apr 9, 2018 • edited Loading

nak3 commented Apr 10, 2018

nak3 commented Apr 10, 2018 • edited Loading

juanvallejo commented Apr 10, 2018

soltysh left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Apr 12, 2018

soltysh commented Apr 12, 2018

nak3 commented Apr 9, 2018 •

edited

Loading

nak3 commented Apr 10, 2018 •

edited

Loading