cleanup #3403: removed --memory, --min-memory. --max-memory, --cpu, --min-cpu, --max-cpu flags from 'odo create' command #3475

devang-gaur · 2020-07-02T09:45:07Z

What type of PR is this?
/kind cleanup

What does does this PR do / why we need it:

Cleans up and remove the memory and cpu flags from odo create command.
There parameters can be set using odo config command.

Which issue(s) this PR fixes:
#3403

How to test changes / Special notes to the reviewer:

openshift-ci-robot · 2020-07-02T09:45:24Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2020-07-02T10:02:09Z

Codecov Report

Merging #3475 into master will increase coverage by 0.12%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #3475      +/-   ##
==========================================
+ Coverage   45.50%   45.63%   +0.12%     
==========================================
  Files         116      122       +6     
  Lines       11712    12247     +535     
==========================================
+ Hits         5330     5589     +259     
- Misses       5862     6107     +245     
- Partials      520      551      +31

Impacted Files	Coverage Δ
pkg/component/component.go	`25.79% <0.00%> (-0.41%)`	⬇️
pkg/odo/util/validation/validators.go	`75.00% <0.00%> (-6.82%)`	⬇️
pkg/envinfo/envinfo.go	`52.08% <0.00%> (-6.98%)`	⬇️
pkg/devfile/adapters/kubernetes/utils/utils.go	`52.80% <0.00%> (-6.69%)`	⬇️
pkg/service/service.go	`41.11% <0.00%> (-4.51%)`	⬇️
pkg/devfile/parser/context/content.go	`82.35% <0.00%> (-2.84%)`	⬇️
...g/devfile/adapters/kubernetes/component/adapter.go	`26.64% <0.00%> (-1.51%)`	⬇️
pkg/catalog/catalog.go	`53.74% <0.00%> (-1.04%)`	⬇️
pkg/occlient/occlient.go	`50.26% <0.00%> (-0.06%)`	⬇️
pkg/kclient/operators.go	`0.00% <0.00%> (ø)`
... and 14 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 cb68a75...f1ff208. Read the comment docs.

prietyc123 · 2020-07-07T04:52:46Z

pkg/odo/cli/component/update.go

+		// Add a disclaimer that we are in *experimental mode*
+		log.Experimental("Experimental mode is enabled, use at your own risk")
+
+		// Configure the context


I think we are talking about configuring the devfile context. Can we change it to Configure the devfile context to be more specific

this change was supposed to be a part of #3453 . I included this here by mistake.

girishramnani · 2020-07-15T08:08:15Z

pkg/odo/cli/component/create.go

@@ -1044,33 +1004,6 @@ func (co *CreateOptions) Run() (err error) {
 	return
 }

-// The general cpu/memory is used as a fallback when it's set and both min-cpu/memory max-cpu/memory are not set
-// when the only thing specified is the min or max value, we exit the application
-func ensureAndLogProperResourceUsage(resource, resourceMin, resourceMax, resourceName string) {


These validations might need to happen at push now, as a user can set them using odo config and then push

Or better yet when these are set using odo config

I added a new commit to add validation for push and config set command. Would that suffice?

prietyc123 · 2020-07-16T07:18:12Z

@dev-gaur It seems a real failure to me

Running odo with args [odo config set MinCPU 0.2 -f]
[odo] Error: mincpu is invalid <nil>
[odo] Usage:
[odo]   odo config set [flags]
[odo] 
[odo] Examples:
[odo] 
[odo]   # Set a configuration value in the local config
[odo]   odo config set Type java
[odo]   odo config set Name test
[odo]   odo config set MinMemory 50M
[odo]   odo config set MaxMemory 500M
[odo]   odo config set Memory 250M
[odo]   odo config set DebugPort 4040
[odo]   odo config set Ignore false
[odo]   odo config set MinCPU 0.5
[odo]   odo config set MaxCPU 2
[odo]   odo config set CPU 1
[odo]   odo config set Ports 8080/TCP,8443/TCP
[odo]   
[odo]   # Set a env variable in the local config
[odo]   odo config set --env KAFKA_HOST=kafka --env KAFKA_PORT=6639
[odo] 
[odo] Flags:
[odo]       --context string   Use given context directory as a source for component settings
[odo]   -e, --env strings      Set the environment variables in config
[odo]   -f, --force            Don't ask for confirmation, set the config directly
[odo]   -h, --help             Help for set
[odo]       --now              Push changes to the cluster immediately
[odo] 
[odo] Additional Flags:
[odo]       --add_dir_header           If true, adds the file directory to the header
[odo]       --log_file string          If non-empty, use this log file
[odo]       --log_file_max_size uint   Defines the maximum size a log file can grow to. Unit is megabytes. If the value is 0, the maximum file size is unlimited. (default 1800)
[odo]       --skip_headers             If true, avoid header prefixes in the log messages
[odo]       --skip_log_headers         If true, avoid headers when opening log files
[odo]   -v, --v Level                  Number for the log level verbosity. Level varies from 0 to 9 (default 0).
[odo]       --vmodule moduleSpec       Comma-separated list of pattern=N settings for file-filtered logging
[odo] 
[odo]  ✗  mincpu is invalid <nil>
Deleting project: umboiofhvz
Running odo with args [odo project delete umboiofhvz -f]
[odo] I0715 23:51:23.621868   24781 application.go:49] Unable to list Service Catalog instances: unable to list ServiceInstances: serviceinstances.servicecatalog.k8s.io is forbidden: User "developer" cannot list serviceinstances.servicecatalog.k8s.io in the namespace "umboiofhvz": no RBAC policy matched
[odo]  ⚠  Warning! Projects are deleted from the cluster asynchronously. Odo does its best to delete the project. Due to multi-tenant clusters, the project may still exist on a different node.
[odo] I0715 23:51:23.645056   24781 odo.go:72] Could not get the latest release information in time. Never mind, exiting gracefully :)
[odo]  ✓  Deleted project : umboiofhvz
Setting current dir to: /home/travis/gopath/src/github.com/openshift/odo/tests/integration
Deleting dir: /tmp/099288889
• Failure [2.015 seconds]
odo preference and config command tests
/home/travis/gopath/src/github.com/openshift/odo/tests/integration/cmd_pref_config_test.go:20
  when creating odo local config in the same config dir
  /home/travis/gopath/src/github.com/openshift/odo/tests/integration/cmd_pref_config_test.go:108
    should set, unset local config successfully [It]
    /home/travis/gopath/src/github.com/openshift/odo/tests/integration/cmd_pref_config_test.go:122
    No future change is possible.  Bailing out early after 0.104s.
    Running odo with args [odo config set MinCPU 0.2 -f]
    Expected
        <int>: 1
    to match exit code:
        <int>: 0

Can you please look into this and also is it a WIP pr, if yes can you please change the title accordingly.

devang-gaur · 2020-07-16T14:13:38Z

@dev-gaur It seems a real failure to me

Running odo with args [odo config set MinCPU 0.2 -f]
[odo] Error: mincpu is invalid <nil>
[odo] Usage:
[odo]   odo config set [flags]
[odo] 
[odo] Examples:
[odo] 
[odo]   # Set a configuration value in the local config
[odo]   odo config set Type java
[odo]   odo config set Name test
[odo]   odo config set MinMemory 50M
[odo]   odo config set MaxMemory 500M
[odo]   odo config set Memory 250M
[odo]   odo config set DebugPort 4040
[odo]   odo config set Ignore false
[odo]   odo config set MinCPU 0.5
[odo]   odo config set MaxCPU 2
[odo]   odo config set CPU 1
[odo]   odo config set Ports 8080/TCP,8443/TCP
[odo]   
[odo]   # Set a env variable in the local config
[odo]   odo config set --env KAFKA_HOST=kafka --env KAFKA_PORT=6639
[odo] 
[odo] Flags:
[odo]       --context string   Use given context directory as a source for component settings
[odo]   -e, --env strings      Set the environment variables in config
[odo]   -f, --force            Don't ask for confirmation, set the config directly
[odo]   -h, --help             Help for set
[odo]       --now              Push changes to the cluster immediately
[odo] 
[odo] Additional Flags:
[odo]       --add_dir_header           If true, adds the file directory to the header
[odo]       --log_file string          If non-empty, use this log file
[odo]       --log_file_max_size uint   Defines the maximum size a log file can grow to. Unit is megabytes. If the value is 0, the maximum file size is unlimited. (default 1800)
[odo]       --skip_headers             If true, avoid header prefixes in the log messages
[odo]       --skip_log_headers         If true, avoid headers when opening log files
[odo]   -v, --v Level                  Number for the log level verbosity. Level varies from 0 to 9 (default 0).
[odo]       --vmodule moduleSpec       Comma-separated list of pattern=N settings for file-filtered logging
[odo] 
[odo]  ✗  mincpu is invalid <nil>
Deleting project: umboiofhvz
Running odo with args [odo project delete umboiofhvz -f]
[odo] I0715 23:51:23.621868   24781 application.go:49] Unable to list Service Catalog instances: unable to list ServiceInstances: serviceinstances.servicecatalog.k8s.io is forbidden: User "developer" cannot list serviceinstances.servicecatalog.k8s.io in the namespace "umboiofhvz": no RBAC policy matched
[odo]  ⚠  Warning! Projects are deleted from the cluster asynchronously. Odo does its best to delete the project. Due to multi-tenant clusters, the project may still exist on a different node.
[odo] I0715 23:51:23.645056   24781 odo.go:72] Could not get the latest release information in time. Never mind, exiting gracefully :)
[odo]  ✓  Deleted project : umboiofhvz
Setting current dir to: /home/travis/gopath/src/github.com/openshift/odo/tests/integration
Deleting dir: /tmp/099288889
• Failure [2.015 seconds]
odo preference and config command tests
/home/travis/gopath/src/github.com/openshift/odo/tests/integration/cmd_pref_config_test.go:20
  when creating odo local config in the same config dir
  /home/travis/gopath/src/github.com/openshift/odo/tests/integration/cmd_pref_config_test.go:108
    should set, unset local config successfully [It]
    /home/travis/gopath/src/github.com/openshift/odo/tests/integration/cmd_pref_config_test.go:122
    No future change is possible.  Bailing out early after 0.104s.
    Running odo with args [odo config set MinCPU 0.2 -f]
    Expected
        <int>: 1
    to match exit code:
        <int>: 0

Can you please look into this and also is it a WIP pr, if yes can you please change the title accordingly.

Hi, sorry I pushed that change in haste without verifying locally as it was trivial. Fixed it now.

girishramnani · 2020-07-20T04:08:30Z

/approve

openshift-ci-robot · 2020-07-20T04:08:46Z

[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

girishramnani · 2020-07-20T04:09:58Z

shouldn't we have tests on config side now to validate improper values @dev-gaur ?

devang-gaur · 2020-07-24T10:41:52Z

/retest

amitkrout · 2020-07-27T11:02:23Z

tests/integration/cmd_push_test.go

+
+			helper.CmdShouldPass("odo", "config", "set", "minmemory", "100Mi", "--context", context)
+			output := helper.CmdShouldFail("odo", "push", "--context", context)
+			Expect(output).To(ContainSubstring("`minmemory` should accompany `maxmemory` or use `odo config set memory` to use same value for both min and max or try not passing any of them"))


Message should be straight forward instead, right.

I would say also in the code implementation make the message pretty straight forward like minmemory should accompany maxmemory, so use `odo config set memory` to use same value for both min and max

amitkrout · 2020-07-27T11:07:57Z

tests/integration/cmd_push_test.go

+			Expect(output).To(ContainSubstring("`minmemory` should accompany `maxmemory` or use `odo config set memory` to use same value for both min and max or try not passing any of them"))
+		})
+
+		It("Should work when both mincpu and maxcpu is set..", func() {


remove it,. It has been covered in config integration test file

amitkrout · 2020-07-27T11:19:03Z

tests/e2escenarios/e2e_images_test.go

@@ -41,7 +41,10 @@ var _ = Describe("odo supported images e2e tests", func() {

 		// create the component
 		helper.CopyExample(filepath.Join("source", srcType), context)
-		helper.CmdShouldPass("odo", "create", cmpType, srcType+"-app", "--project", project, "--context", context, "--app", appName, "--min-memory", "400Mi", "--max-memory", "700Mi")
+		helper.CmdShouldPass("odo", "create", cmpType, srcType+"-app", "--project", project, "--context", context, "--app", appName)


e2e test stands for broader user specific scenario. These validation can be done through UTs or integration test label. So please remove those lines.

Ofcourse if we find a user specific scenario where we have such hard requirement of adding those validation then we will add these line for sure.

If I don't trim the memory and cpu flags from here then this will fail.

Ahh...got it. I did not realize it. Sorry for the false alarm.

amitkrout · 2020-07-27T11:20:10Z

tests/integration/cmd_push_test.go

@@ -53,6 +53,94 @@ var _ = Describe("odo push command tests", func() {

 	})

+	Context("Check memory and cpu config before odo push", func() {
+		It("Should work when both minmemory and maxmemory is set..", func() {


Remove it. It has been covered in config integration test file.

amitkrout · 2020-07-27T11:22:45Z

tests/integration/cmd_push_test.go

+			Expect(output).To(ContainSubstring("`minmemory` should accompany `maxmemory` or use `odo config set memory` to use same value for both min and max or try not passing any of them"))
+		})
+
+		It("should fail if maxmemory is set but minmemory is not set..", func() {


Can these validation be done through UTs instead of over crowding the integration test file ?

Since it covers multiple packages. Not just config package. It cannot be covered in UTs.

The memory and cpu is set in config package but the overall/e2e validation of config parameters occur in component push package

amitkrout · 2020-07-27T11:26:35Z

tests/integration/cmd_push_test.go

+			helper.CmdShouldPass("odo", "push", "--context", context)
+		})
+
+		It("Should work when cpu is set", func() {


Cna it be done through code level validation i mean through UTs ?

No. I don't think so, due to the same reason I mentioned above.

amitkrout · 2020-07-27T11:27:46Z

tests/integration/cmd_push_test.go

+			helper.CmdShouldPass("odo", "push", "--context", context)
+		})
+
+		It("Should fail if mincpu is set but maxcpu is not set..", func() {


Can it be done through code level validation i mean through UTs ?

amitkrout · 2020-07-27T11:28:07Z

tests/integration/cmd_push_test.go

+			Expect(output).To(ContainSubstring("`mincpu` should accompany `maxcpu` or use `odo config set cpu` to use same value for both min and max or try not passing any of them"))
+		})
+
+		It("should fail if maxcpu is set but mincpu is not set..", func() {


Can it be done through code level validation i mean through UTs ?

amitkrout

@dev-gaur Theses flags are applicable for devfile component aswell, so can you please test one negative and one happy path scenario with devfile component push.

devang-gaur · 2020-07-28T14:37:02Z

/retest

dharmit · 2020-07-29T09:40:38Z

$ odo config set MinMemory -1M
Error: unknown shorthand flag: '1' in -1M

$ odo config set MinMemory "-1M"
Error: unknown shorthand flag: '1' in -1M

$ odo version
odo v1.2.4 (f1ff2087e)

I'm expecting the function at https://github.com/openshift/odo/pull/3475/files#diff-cf69f55cdfd58073682104f12e305dd8R45 to show me a proper error message. Am I doint something wrong?

devang-gaur · 2020-07-29T10:19:57Z

$ odo config set MinMemory -1M
Error: unknown shorthand flag: '1' in -1M

$ odo config set MinMemory "-1M"
Error: unknown shorthand flag: '1' in -1M

$ odo version
odo v1.2.4 (f1ff2087e)
I'm expecting the function at https://github.com/openshift/odo/pull/3475/files#diff-cf69f55cdfd58073682104f12e305dd8R45 to show me a proper error message. Am I doint something wrong?

This message isn't coming from the odo code, but the package that we have imported to detect flags to the cli. Its detecting -1 as a provided flag value. I'm not sure how can I override that.

dharmit · 2020-07-29T10:35:31Z

Then which scenario(s) does NonNegativeValidator really handle?

devang-gaur · 2020-07-29T11:25:56Z

Then which scenario(s) does NonNegativeValidator really handle?

I just added it as an additional measure. If somehow user managers to override the package and pass the negative values.

dharmit

/lgtm

openshift-bot · 2020-07-29T17:55:02Z

/retest