Skip to content

Commit f83dfd7

Browse files
openshift-merge-robotopenshift-merge-robot
authoredApr 12, 2018
Merge pull request #19169 from nak3/set-volume
Change the order of Validate() and Complete() for oc set volume
·
v4.1.0v3.10.0-rc.0
2 parents 516f31f + ebd7209 commit f83dfd7

File tree

2 files changed

+144
-137
lines changed

2 files changed

+144
-137
lines changed
 

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

+143-136
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,11 @@ type AddVolumeOptions struct {
155155

156156
func NewCmdVolume(fullName string, f *clientcmd.Factory, out, errOut io.Writer) *cobra.Command {
157157
addOpts := &AddVolumeOptions{}
158-
opts := &VolumeOptions{AddOpts: addOpts}
158+
opts := &VolumeOptions{
159+
AddOpts: addOpts,
160+
Out: out,
161+
Err: errOut,
162+
}
159163
cmd := &cobra.Command{
160164
Use: "volumes RESOURCE/NAME --add|--remove|--list",
161165
Short: "Update volumes on a pod template",
@@ -165,12 +169,12 @@ func NewCmdVolume(fullName string, f *clientcmd.Factory, out, errOut io.Writer)
165169
Run: func(cmd *cobra.Command, args []string) {
166170
addOpts.TypeChanged = cmd.Flag("type").Changed
167171

172+
kcmdutil.CheckErr(opts.Complete(f, cmd))
173+
168174
err := opts.Validate(cmd, args)
169175
if err != nil {
170176
kcmdutil.CheckErr(kcmdutil.UsageErrorf(cmd, err.Error()))
171177
}
172-
err = opts.Complete(f, cmd, out, errOut)
173-
kcmdutil.CheckErr(err)
174178

175179
err = opts.RunVolume(args, f)
176180
if err == kcmdutil.ErrExit {
@@ -228,32 +232,19 @@ func (v *VolumeOptions) Validate(cmd *cobra.Command, args []string) error {
228232
return errors.New("provide one or more resources to add, list, or delete volumes on as TYPE/NAME")
229233
}
230234

231-
numOps := 0
232-
if v.Add {
233-
numOps++
234-
}
235-
if v.Remove {
236-
numOps++
237-
}
238-
if v.List {
239-
numOps++
240-
}
241-
242-
switch {
243-
case numOps == 0:
244-
v.List = true
245-
case numOps > 1:
246-
return errors.New("you may only specify one operation at a time")
247-
}
248-
249-
output := kcmdutil.GetFlagString(cmd, "output")
250-
if v.List && len(output) > 0 {
235+
if v.List && len(v.Output) > 0 {
251236
return errors.New("--list and --output may not be specified together")
252237
}
253238

254-
err := v.AddOpts.Validate(v.Add)
255-
if err != nil {
256-
return err
239+
if v.Add {
240+
err := v.AddOpts.Validate()
241+
if err != nil {
242+
return err
243+
}
244+
} else if len(v.AddOpts.Source) > 0 || len(v.AddOpts.Path) > 0 || len(v.AddOpts.SecretName) > 0 ||
245+
len(v.AddOpts.ConfigMapName) > 0 || len(v.AddOpts.ClaimName) > 0 || len(v.AddOpts.DefaultMode) > 0 ||
246+
v.AddOpts.Overwrite {
247+
return errors.New("--type|--path|--configmap-name|--secret-name|--claim-name|--source|--default-mode|--overwrite are only valid for --add operation")
257248
}
258249
// Removing all volumes for the resource type needs confirmation
259250
if v.Remove && len(v.Name) == 0 && !v.Confirm {
@@ -262,111 +253,74 @@ func (v *VolumeOptions) Validate(cmd *cobra.Command, args []string) error {
262253
return nil
263254
}
264255

265-
func (a *AddVolumeOptions) Validate(isAddOp bool) error {
266-
if isAddOp {
267-
if len(a.Type) == 0 && (len(a.ClaimName) > 0 || len(a.ClaimSize) > 0) {
268-
a.Type = "persistentvolumeclaim"
269-
a.TypeChanged = true
270-
}
271-
if len(a.Type) == 0 && (len(a.SecretName) > 0) {
272-
a.Type = "secret"
273-
a.TypeChanged = true
274-
}
275-
if len(a.Type) == 0 && (len(a.ConfigMapName) > 0) {
276-
a.Type = "configmap"
277-
a.TypeChanged = true
278-
}
279-
if len(a.Type) == 0 && (len(a.Path) > 0) {
280-
a.Type = "hostpath"
281-
a.TypeChanged = true
282-
}
283-
if len(a.Type) == 0 {
284-
a.Type = "emptydir"
285-
}
286-
287-
if len(a.Type) == 0 && len(a.Source) == 0 {
288-
return errors.New("must provide --type or --source for --add operation")
289-
} else if a.TypeChanged && len(a.Source) > 0 {
290-
return errors.New("either specify --type or --source but not both for --add operation")
291-
}
256+
func (a *AddVolumeOptions) Validate() error {
257+
if len(a.Type) == 0 && len(a.Source) == 0 {
258+
return errors.New("must provide --type or --source for --add operation")
259+
} else if a.TypeChanged && len(a.Source) > 0 {
260+
return errors.New("either specify --type or --source but not both for --add operation")
261+
}
292262

293-
if len(a.Type) > 0 {
294-
switch strings.ToLower(a.Type) {
295-
case "emptydir":
296-
if len(a.DefaultMode) > 0 {
297-
return errors.New("--default-mode is only available for secrets and configmaps")
298-
}
299-
case "hostpath":
300-
if len(a.Path) == 0 {
301-
return errors.New("must provide --path for --type=hostPath")
302-
}
303-
if len(a.DefaultMode) > 0 {
304-
return errors.New("--default-mode is only available for secrets and configmaps")
305-
}
306-
case "secret":
307-
if len(a.SecretName) == 0 {
308-
return errors.New("must provide --secret-name for --type=secret")
309-
}
310-
if len(a.DefaultMode) > 0 {
311-
if ok, _ := regexp.MatchString(`\b0?[0-7]{3}\b`, a.DefaultMode); !ok {
312-
return errors.New("--default-mode must be between 0000 and 0777")
313-
}
314-
}
315-
case "configmap":
316-
if len(a.ConfigMapName) == 0 {
317-
return errors.New("must provide --configmap-name for --type=configmap")
318-
}
319-
if len(a.DefaultMode) > 0 {
320-
if ok, _ := regexp.MatchString(`\b0?[0-7]{3}\b`, a.DefaultMode); !ok {
321-
return errors.New("--default-mode must be between 0000 and 0777")
322-
}
323-
}
324-
case "persistentvolumeclaim", "pvc":
325-
if len(a.ClaimName) == 0 && len(a.ClaimSize) == 0 {
326-
return errors.New("must provide --claim-name or --claim-size (to create a new claim) for --type=pvc")
327-
}
328-
if len(a.DefaultMode) > 0 {
329-
return errors.New("--default-mode is only available for secrets and configmaps")
330-
}
331-
default:
332-
return errors.New("invalid volume type. Supported types: emptyDir, hostPath, secret, persistentVolumeClaim")
263+
if len(a.Type) > 0 {
264+
switch strings.ToLower(a.Type) {
265+
case "emptydir":
266+
case "hostpath":
267+
if len(a.Path) == 0 {
268+
return errors.New("must provide --path for --type=hostPath")
333269
}
334-
} else if len(a.Path) > 0 || len(a.SecretName) > 0 || len(a.ClaimName) > 0 {
335-
return errors.New("--path|--secret-name|--claim-name are only valid for --type option")
336-
}
337-
338-
if len(a.Source) > 0 {
339-
var source map[string]interface{}
340-
err := json.Unmarshal([]byte(a.Source), &source)
341-
if err != nil {
342-
return err
270+
case "secret":
271+
if len(a.SecretName) == 0 {
272+
return errors.New("must provide --secret-name for --type=secret")
343273
}
344-
if len(source) > 1 {
345-
return errors.New("must provide only one volume for --source")
274+
if ok, _ := regexp.MatchString(`\b0?[0-7]{3}\b`, a.DefaultMode); !ok {
275+
return errors.New("--default-mode must be between 0000 and 0777")
346276
}
347-
348-
var vs kapi.VolumeSource
349-
err = json.Unmarshal([]byte(a.Source), &vs)
350-
if err != nil {
351-
return err
277+
case "configmap":
278+
if len(a.ConfigMapName) == 0 {
279+
return errors.New("must provide --configmap-name for --type=configmap")
352280
}
353-
}
354-
if len(a.ClaimClass) > 0 {
355-
selectedLowerType := strings.ToLower(a.Type)
356-
if selectedLowerType != "persistentvolumeclaim" && selectedLowerType != "pvc" {
357-
return errors.New("must provide --type as persistentVolumeClaim")
281+
if ok, _ := regexp.MatchString(`\b0?[0-7]{3}\b`, a.DefaultMode); !ok {
282+
return errors.New("--default-mode must be between 0000 and 0777")
358283
}
359-
if len(a.ClaimSize) == 0 {
360-
return errors.New("must provide --claim-size to create new pvc with claim-class")
284+
case "persistentvolumeclaim", "pvc":
285+
if len(a.ClaimName) == 0 && len(a.ClaimSize) == 0 {
286+
return errors.New("must provide --claim-name or --claim-size (to create a new claim) for --type=pvc")
361287
}
288+
default:
289+
return errors.New("invalid volume type. Supported types: emptyDir, hostPath, secret, persistentVolumeClaim")
290+
}
291+
} else if len(a.Path) > 0 || len(a.SecretName) > 0 || len(a.ClaimName) > 0 {
292+
return errors.New("--path|--secret-name|--claim-name are only valid for --type option")
293+
}
294+
295+
if len(a.Source) > 0 {
296+
var source map[string]interface{}
297+
err := json.Unmarshal([]byte(a.Source), &source)
298+
if err != nil {
299+
return err
300+
}
301+
if len(source) > 1 {
302+
return errors.New("must provide only one volume for --source")
303+
}
304+
305+
var vs kapi.VolumeSource
306+
err = json.Unmarshal([]byte(a.Source), &vs)
307+
if err != nil {
308+
return err
309+
}
310+
}
311+
if len(a.ClaimClass) > 0 {
312+
selectedLowerType := strings.ToLower(a.Type)
313+
if selectedLowerType != "persistentvolumeclaim" && selectedLowerType != "pvc" {
314+
return errors.New("must provide --type as persistentVolumeClaim")
315+
}
316+
if len(a.ClaimSize) == 0 {
317+
return errors.New("must provide --claim-size to create new pvc with claim-class")
362318
}
363-
} else if len(a.Source) > 0 || len(a.Path) > 0 || len(a.SecretName) > 0 || len(a.ConfigMapName) > 0 || len(a.ClaimName) > 0 || a.Overwrite {
364-
return errors.New("--type|--path|--configmap-name|--secret-name|--claim-name|--source|--overwrite are only valid for --add operation")
365319
}
366320
return nil
367321
}
368322

369-
func (v *VolumeOptions) Complete(f *clientcmd.Factory, cmd *cobra.Command, out, errOut io.Writer) error {
323+
func (v *VolumeOptions) Complete(f *clientcmd.Factory, cmd *cobra.Command) error {
370324
kc, err := f.ClientSet()
371325
if err != nil {
372326
return err
@@ -379,6 +333,24 @@ func (v *VolumeOptions) Complete(f *clientcmd.Factory, cmd *cobra.Command, out,
379333
}
380334
mapper, typer := f.Object()
381335

336+
numOps := 0
337+
if v.Add {
338+
numOps++
339+
}
340+
if v.Remove {
341+
numOps++
342+
}
343+
if v.List {
344+
numOps++
345+
}
346+
347+
switch {
348+
case numOps == 0:
349+
v.List = true
350+
case numOps > 1:
351+
return errors.New("you may only specify one operation at a time")
352+
}
353+
382354
v.Output = kcmdutil.GetFlagString(cmd, "output")
383355
v.PrintObject = func(infos []*resource.Info) error {
384356
return f.PrintResourceInfos(cmd, v.Local, infos, v.Out)
@@ -387,44 +359,79 @@ func (v *VolumeOptions) Complete(f *clientcmd.Factory, cmd *cobra.Command, out,
387359
v.Cmd = cmd
388360
v.DefaultNamespace = cmdNamespace
389361
v.ExplicitNamespace = explicit
390-
v.Out = out
391-
v.Err = errOut
392362
v.Mapper = mapper
393363
v.Typer = typer
394364
v.CategoryExpander = f.CategoryExpander()
395365
v.RESTClientFactory = f.ClientForMapping
396366
v.UpdatePodSpecForObject = f.UpdatePodSpecForObject
397367
v.Encoder = kcmdutil.InternalVersionJSONEncoder()
398368

369+
// Complete AddOpts
370+
if v.Add {
371+
err := v.AddOpts.Complete()
372+
if err != nil {
373+
return err
374+
}
375+
}
376+
return nil
377+
}
378+
379+
func (a *AddVolumeOptions) Complete() error {
380+
if len(a.Type) == 0 {
381+
switch {
382+
case len(a.ClaimName) > 0 || len(a.ClaimSize) > 0:
383+
a.Type = "persistentvolumeclaim"
384+
a.TypeChanged = true
385+
case len(a.SecretName) > 0:
386+
a.Type = "secret"
387+
a.TypeChanged = true
388+
case len(a.ConfigMapName) > 0:
389+
a.Type = "configmap"
390+
a.TypeChanged = true
391+
case len(a.Path) > 0:
392+
a.Type = "hostpath"
393+
a.TypeChanged = true
394+
default:
395+
a.Type = "emptydir"
396+
}
397+
}
398+
if a.Type == "configmap" || a.Type == "secret" {
399+
if len(a.DefaultMode) == 0 {
400+
a.DefaultMode = "644"
401+
}
402+
} else {
403+
if len(a.DefaultMode) != 0 {
404+
return errors.New("--default-mode is only available for secrets and configmaps")
405+
}
406+
}
407+
399408
// In case of volume source ignore the default volume type
400-
if len(v.AddOpts.Source) > 0 {
401-
v.AddOpts.Type = ""
409+
if len(a.Source) > 0 {
410+
a.Type = ""
402411
}
403-
if len(v.AddOpts.ClaimSize) > 0 {
404-
v.AddOpts.CreateClaim = true
405-
if len(v.AddOpts.ClaimName) == 0 {
406-
v.AddOpts.ClaimName = names.SimpleNameGenerator.GenerateName("pvc-")
412+
if len(a.ClaimSize) > 0 {
413+
a.CreateClaim = true
414+
if len(a.ClaimName) == 0 {
415+
a.ClaimName = names.SimpleNameGenerator.GenerateName("pvc-")
407416
}
408-
q, err := kresource.ParseQuantity(v.AddOpts.ClaimSize)
417+
q, err := kresource.ParseQuantity(a.ClaimSize)
409418
if err != nil {
410419
return fmt.Errorf("--claim-size is not valid: %v", err)
411420
}
412-
v.AddOpts.ClaimSize = q.String()
413-
}
414-
if len(v.AddOpts.DefaultMode) == 0 {
415-
v.AddOpts.DefaultMode = "644"
421+
a.ClaimSize = q.String()
416422
}
417-
switch strings.ToLower(v.AddOpts.ClaimMode) {
423+
switch strings.ToLower(a.ClaimMode) {
418424
case strings.ToLower(string(kapi.ReadOnlyMany)), "rom":
419-
v.AddOpts.ClaimMode = string(kapi.ReadOnlyMany)
425+
a.ClaimMode = string(kapi.ReadOnlyMany)
420426
case strings.ToLower(string(kapi.ReadWriteOnce)), "rwo":
421-
v.AddOpts.ClaimMode = string(kapi.ReadWriteOnce)
427+
a.ClaimMode = string(kapi.ReadWriteOnce)
422428
case strings.ToLower(string(kapi.ReadWriteMany)), "rwm":
423-
v.AddOpts.ClaimMode = string(kapi.ReadWriteMany)
429+
a.ClaimMode = string(kapi.ReadWriteMany)
424430
case "":
425431
default:
426432
return errors.New("--claim-mode must be one of ReadWriteOnce (rwo), ReadWriteMany (rwm), or ReadOnlyMany (rom)")
427433
}
434+
428435
return nil
429436
}
430437

‎pkg/oc/cli/cmd/set/volume_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -346,7 +346,7 @@ func TestValidateAddOptions(t *testing.T) {
346346

347347
for _, testCase := range tests {
348348
addOpts := testCase.addOpts
349-
err := addOpts.Validate(true)
349+
err := addOpts.Validate()
350350
if testCase.expectedError == nil && err != nil {
351351
t.Errorf("Expected nil error for %s got %s", testCase.name, err)
352352
continue

0 commit comments

Comments
 (0)
Please sign in to comment.