Add validation for parameters passed with command #3259

devang-gaur · 2020-05-28T10:37:56Z

kind bug

What does does this PR do / why we need it:
Adds Validation on parameters passed with odo config set commands

Which issue(s) this PR fixes:

dharmit · 2020-05-29T06:12:55Z

Couple of things:

$ odo config set ports "8080/TCP/UDP" -f
E0529 11:36:57.513401    6790 config.go:690] Validation failed for the provided arguments, Error: unable to parse the port string 8080/TCP/UDP
 ✗  unknown parameter :'' is not a parameter in local odo config

IMO, we should simply print the error message. Details about error number, line where it occurred, etc. should not be shown to the user.

Also, why is it showing the error unknown parameter :'' is not a parameter in local odo config? I'm little confused there. Error: unable to parse the port string 8080/TCP/UDP looks good, however.

IMO, below should fail because I've not provided TCP or UDP to port 9090:

$ odo config set ports "8080/TCP,9090" -f
 ✓  Local config successfully updated

Run `odo push --config` command to apply changes to the cluster

$ odo config view
COMPONENT SETTINGS
------------------------------------------------
PARAMETER         CURRENT_VALUE
Type              nodejs
Application       app
Project           default
SourceType        local
Ref               
SourceLocation    ./
Ports             8080/TCP,9090
Name              nodejs-nodejs-ex-wcxj
MinMemory         
MaxMemory         
DebugPort         
Ignore            
MinCPU            
MaxCPU

Another thing from code point of view - please add a comment describing what IsValidLArgumentList is doing.

devang-gaur · 2020-05-29T11:40:25Z

@dharmit

odo config set ports "8080/TCP,9090" -f

is not failing because the port validator function accepts it.

I'm using existing validation utilities so that the validation behavior remains uniform. Should I create a different validator ?

https://github.com/openshift/odo/blob/6bd0d73dc67e014e34f7e92a35dab26734163afc/pkg/odo/util/validation/validators.go#L60

dharmit · 2020-06-01T11:13:01Z

Should I create a different validator ?

Nope. My bad. Should have read the comments that said it's a valid value.

girishramnani · 2020-06-01T11:17:11Z

If I remember correctly not providing the protocol in <PORT>TCP/UDP defaults to TCP I think in most tools

dharmit · 2020-06-01T11:26:54Z

If I remember correctly not providing the protocol in <PORT>TCP/UDP defaults to TCP I think in most tools

Behind the scenes, it does. I tried creating a URL for port without protocol info in config.yaml and it works. It doesn't show up in odo config view, however. Not an issue, IMO.

devang-gaur · 2020-06-01T11:32:04Z

If I remember correctly not providing the protocol in <PORT>TCP/UDP defaults to TCP I think in most tools

Behind the scenes, it does. I tried creating a URL for port without protocol info in config.yaml and it works. It doesn't show up in odo config view, however. Not an issue, IMO.

I think it would help to be clear with the user if we specify that in odo config view

dharmit · 2020-06-03T10:26:55Z

I think it would help to be clear with the user if we specify that in odo config view

Or, it could be mentioned in the CLI documentation or actual documentation. If we'd want to make it clear in the output of odo config view, we would have to modify the validation function.

girishramnani · 2020-06-10T08:45:36Z

pkg/config/config.go

+	case "ports", "debugport":
+		err = validation.PortsValidator(args[1])
+	default:
+		if len(args) < 2 || len(args[1]) == 0 {


Shouldn’t this check be at the top?

girishramnani · 2020-06-10T08:47:32Z

pkg/config/config.go

+	var err error
+	switch param {
+	case "ports", "debugport":
+		err = validation.PortsValidator(args[1])


What would happen if I provided odo config set ports without any value for the ports? Probably would panic due to args[1] I guess

There's a check for this https://github.com/openshift/odo/pull/3259/files#diff-091ace02b49a46f55f2586cfec89a160R737

girishramnani · 2020-06-10T08:51:32Z

pkg/config/config.go

+	param, ok := asLocallySupportedParameter(args[0])
+
+	if !ok {
+		return errors.Errorf("the parameter provided : %v, is not present in the supported parameters list.", args[0])


There is an error string convention that error don’t start with capitals and doesn’t end with a fullstop

metacosm · 2020-06-11T15:19:04Z

pkg/config/config.go

+	param, ok := asLocallySupportedParameter(args[0])
+
+	if !ok {
+		err = errors.Errorf("the parameter provided : %v, is not present in the supported parameters list", args[0])


I would just say: "provided parameter '%v' is not a supported parameter"

metacosm · 2020-06-11T15:20:08Z

pkg/config/config.go

+	if !ok {
+		err = errors.Errorf("the parameter provided : %v, is not present in the supported parameters list", args[0])
+	} else if len(args) < 2 || len(args[1]) == 0 {
+		err = errors.Errorf("no argument provided for parameter %v", param)


"no value" instead of "argument"

metacosm · 2020-06-11T15:22:05Z

pkg/config/config.go

+	}
+
+	if err != nil {
+		err = errors.Errorf("validation failed for the provided arguments, Error: %v", err)


I would output error as "validation failed for the provided arguments: %v"

metacosm · 2020-06-11T16:38:44Z

pkg/config/config.go

@@ -730,3 +731,30 @@ func (lci *LocalConfigInfo) GetOSSourcePath() (path string, err error) {

 	return sourceOSPath, err
 }
+
+// IsValidArgumentList validates the argument list for `odo config` command
+func IsValidArgumentList(args []string) error {


This function should actually be merged with the Args function that is passed to the command struct when it's created since otherwise the check on args length will never occur (as it's done in that function). Actually, there would be merits to check the args there instead of in Complete… See: https://github.com/openshift/odo/blob/6bd0d73dc67e014e34f7e92a35dab26734163afc/pkg/odo/cli/config/set.go#L96-L103

girishramnani · 2020-06-16T04:19:10Z

@dev-gaur CI failing

girishramnani · 2020-06-19T17:25:54Z

CI seem to fail but code is good hence, please fix the tests if need be.
/approve

openshift-ci-robot · 2020-06-19T17:26:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: girishramnani

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:

~~OWNERS~~ [girishramnani]

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

codecov · 2020-06-22T08:38:52Z

Codecov Report

Merging #3259 into master will increase coverage by 0.16%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3259      +/-   ##
==========================================
+ Coverage   46.28%   46.45%   +0.16%     
==========================================
  Files         112      112              
  Lines       11200    11237      +37     
==========================================
+ Hits         5184     5220      +36     
+ Misses       5518     5513       -5     
- Partials      498      504       +6

Impacted Files	Coverage Δ
pkg/config/config.go	`45.64% <100.00%> (ø)`
pkg/sync/sync.go	`43.56% <0.00%> (-4.96%)`	⬇️
pkg/util/file_indexer.go	`7.50% <0.00%> (-2.50%)`	⬇️
pkg/devfile/adapters/kubernetes/utils/utils.go	`52.48% <0.00%> (-1.93%)`	⬇️
pkg/devfile/adapters/docker/component/adapter.go	`74.01% <0.00%> (-0.99%)`	⬇️
pkg/devfile/adapters/docker/component/utils.go	`68.83% <0.00%> (-0.91%)`	⬇️
pkg/preference/preference.go	`61.87% <0.00%> (-0.63%)`	⬇️
pkg/lclient/client.go	`0.00% <0.00%> (ø)`
pkg/lclient/fakeclient.go	`82.11% <0.00%> (ø)`
pkg/testingutil/devfile.go	`0.00% <0.00%> (ø)`
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f736e3b...d6bf379. Read the comment docs.

mohammedzee1000 · 2020-06-30T07:56:34Z

/kind bug

amitkrout · 2020-06-30T10:04:38Z

pkg/config/config.go

+		err = errors.Errorf("the parameter provided : %v, is not supported", args[0])
+	}
+
+	switch param {


+100, I feel happy when i see switch statement instead of multiple if else

metacosm · 2020-06-30T10:07:52Z

pkg/config/config.go

@@ -730,3 +731,31 @@ func (lci *LocalConfigInfo) GetOSSourcePath() (path string, err error) {

 	return sourceOSPath, err
 }
+
+// IsValidArgumentList validates the argument list for `odo config` command
+func IsValidArgumentList(args []string) error {


This function should not be in that file as it only deals with command-related code, not config-related functionality per se. My advice would be to put it with the set command and unexport it.

metacosm · 2020-06-30T10:08:59Z

pkg/config/config.go

+	param, ok := asLocallySupportedParameter(args[0])
+
+	if !ok {
+		err = errors.Errorf("the parameter provided : %v, is not supported", args[0])


The wording is awkward. See my previous comment on this topic.

metacosm · 2020-06-30T10:09:37Z

pkg/config/config.go

+	}
+
+	var err error
+	param, ok := asLocallySupportedParameter(args[0])


Moving the enclosing function to the set command will require exporting this function.

girishramnani · 2020-07-02T19:45:09Z

@dev-gaur any update on the comments made by chris?

metacosm

LGTM

girishramnani · 2020-07-04T14:44:47Z

/retest

openshift-bot · 2020-07-04T15:27:43Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-04T16:32:44Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-04T16:45:42Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-04T21:57:42Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot requested review from girishramnani and mik-dass May 28, 2020 10:38

devang-gaur mentioned this pull request May 28, 2020

"odo config set" doesn't validate port values and thus "odo describe" panics #3036

Closed

devang-gaur force-pushed the validate_params branch from 43240e7 to e431cda Compare May 28, 2020 10:45

devang-gaur force-pushed the validate_params branch from d0c00bd to 914d9c9 Compare May 29, 2020 11:02

devang-gaur force-pushed the validate_params branch 3 times, most recently from d74ba3f to 5ad2ab2 Compare May 29, 2020 12:35

devang-gaur force-pushed the validate_params branch from 5ad2ab2 to 313fab9 Compare June 3, 2020 10:36

girishramnani reviewed Jun 10, 2020

View reviewed changes

devang-gaur force-pushed the validate_params branch from 313fab9 to eb44e8a Compare June 11, 2020 12:14

metacosm suggested changes Jun 11, 2020

View reviewed changes

openshift-ci-robot assigned metacosm Jun 11, 2020

snowdrop-bot mentioned this pull request Jun 11, 2020

Add validation for parameters passed with command snowdrop-zen/odo#2

Closed

metacosm reviewed Jun 11, 2020

View reviewed changes

metacosm mentioned this pull request Jun 11, 2020

odo PR reviews snowdrop/team#113

Closed

devang-gaur force-pushed the validate_params branch from eb44e8a to 5fcdb29 Compare June 18, 2020 10:50

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jun 19, 2020

devang-gaur force-pushed the validate_params branch from 5fcdb29 to 804be26 Compare June 22, 2020 05:44

openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jun 30, 2020

devang-gaur requested review from metacosm and girishramnani June 30, 2020 08:07

amitkrout reviewed Jun 30, 2020

View reviewed changes

metacosm suggested changes Jun 30, 2020

View reviewed changes

Add validation for parameters passed with 'odo config set' command

d6bf379

devang-gaur force-pushed the validate_params branch from 804be26 to d6bf379 Compare July 3, 2020 12:08

metacosm approved these changes Jul 4, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Jul 4, 2020

openshift-merge-robot merged commit c8bff4b into redhat-developer:master Jul 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validation for parameters passed with command #3259

Add validation for parameters passed with command #3259

devang-gaur commented May 28, 2020

dharmit commented May 29, 2020

devang-gaur commented May 29, 2020

dharmit commented Jun 1, 2020

girishramnani commented Jun 1, 2020 •

edited

Loading

dharmit commented Jun 1, 2020

devang-gaur commented Jun 1, 2020 •

edited

Loading

dharmit commented Jun 3, 2020

girishramnani Jun 10, 2020

girishramnani Jun 10, 2020 •

edited

Loading

devang-gaur Jun 11, 2020

girishramnani Jun 10, 2020 •

edited

Loading

metacosm Jun 11, 2020

metacosm Jun 11, 2020

metacosm Jun 11, 2020

metacosm Jun 11, 2020 •

edited

Loading

girishramnani commented Jun 16, 2020

girishramnani commented Jun 19, 2020

openshift-ci-robot commented Jun 19, 2020

codecov bot commented Jun 22, 2020 •

edited

Loading

mohammedzee1000 commented Jun 30, 2020

amitkrout Jun 30, 2020

metacosm Jun 30, 2020

metacosm Jun 30, 2020

metacosm Jun 30, 2020

girishramnani commented Jul 2, 2020

metacosm left a comment

girishramnani commented Jul 4, 2020

openshift-bot commented Jul 4, 2020

openshift-bot commented Jul 4, 2020

openshift-bot commented Jul 4, 2020

openshift-bot commented Jul 4, 2020

Add validation for parameters passed with command #3259

Add validation for parameters passed with command #3259

Conversation

devang-gaur commented May 28, 2020

dharmit commented May 29, 2020

devang-gaur commented May 29, 2020

dharmit commented Jun 1, 2020

girishramnani commented Jun 1, 2020 • edited Loading

dharmit commented Jun 1, 2020

devang-gaur commented Jun 1, 2020 • edited Loading

dharmit commented Jun 3, 2020

Choose a reason for hiding this comment

girishramnani Jun 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girishramnani Jun 10, 2020 • edited Loading

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

metacosm Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

girishramnani commented Jun 16, 2020

girishramnani commented Jun 19, 2020

openshift-ci-robot commented Jun 19, 2020

codecov bot commented Jun 22, 2020 • edited Loading

Codecov Report

mohammedzee1000 commented Jun 30, 2020

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

girishramnani commented Jul 2, 2020

metacosm left a comment

Choose a reason for hiding this comment

girishramnani commented Jul 4, 2020

openshift-bot commented Jul 4, 2020

openshift-bot commented Jul 4, 2020

openshift-bot commented Jul 4, 2020

openshift-bot commented Jul 4, 2020

girishramnani commented Jun 1, 2020 •

edited

Loading

devang-gaur commented Jun 1, 2020 •

edited

Loading

girishramnani Jun 10, 2020 •

edited

Loading

girishramnani Jun 10, 2020 •

edited

Loading

metacosm Jun 11, 2020 •

edited

Loading

codecov bot commented Jun 22, 2020 •

edited

Loading