Refactor utils command to follow Complete/Validate/Run pattern #1264

metacosm · 2019-01-31T14:36:01Z

Was the change discussed in an issue?

How to test changes?

Play with odo utils commands.

codeclimate · 2019-01-31T14:36:52Z

Code Climate has analyzed commit f9e6912 and detected 9 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Duplication	2
Style	7

View more on Code Climate.

AppVeyorBot · 2019-01-31T16:28:52Z

✅ odo build 1314 completed and artifacts can be found here (for commit 0bb0e94986 by @metacosm)

AppVeyorBot · 2019-01-31T19:52:08Z

✅ odo build 1319 completed and artifacts can be found here (for commit 65b605f3d8 by @metacosm)

codecov · 2019-01-31T21:56:51Z

Codecov Report

Merging #1264 into master will decrease coverage by 0.12%.
The diff coverage is 50.63%.

@@            Coverage Diff             @@
##           master    #1264      +/-   ##
==========================================
- Coverage    42.8%   42.67%   -0.13%     
==========================================
  Files          38       40       +2     
  Lines        4841     4907      +66     
==========================================
+ Hits         2072     2094      +22     
- Misses       2564     2608      +44     
  Partials      205      205

Impacted Files	Coverage Δ
pkg/odo/cli/utils/utils.go	`0% <0%> (ø)`
pkg/util/util.go	`64.78% <100%> (+1.37%)`	⬆️
pkg/odo/cli/utils/terminal.go	`7.14% <7.14%> (ø)`
pkg/config/config.go	`67.17% <93.75%> (+2.88%)`	⬆️
pkg/component/watch.go	`67.92% <0%> (-2.52%)`	⬇️

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 5f2be30...f9e6912. Read the comment docs.

geoand · 2019-02-01T06:46:33Z

I haven't really ever used odo utils so I'm probably not the best person to review :(

anmolbabu · 2019-02-01T13:53:08Z

pkg/config/config.go

+var (
+	// records information on supported parameters
+	supportedParameterDescriptions = map[string]string{
+		"UpdateNotification": "Controls if an update notification is shown or not (true or false)",


Can these configurables be string constants?
I see them being referred in more than 1 places which might be prone to bugs
So, this then converts to map[string_alias]string

Let me see what I can do.

Extracted constants.

anmolbabu · 2019-02-01T13:54:09Z

pkg/config/config.go

@@ -141,29 +142,31 @@ func (c *ConfigInfo) writeToFile() error {
 // SetConfiguration modifies Odo configurations in the config file
 // as of now being used for nameprefix, timeout, updatenotification
 func (c *ConfigInfo) SetConfiguration(parameter string, value string) error {
-	// processing values according to the parameter names
-	switch parameter {
+	if p, ok := asSupportedParameter(parameter); ok {


I tend to feel config or configurable would be apt instead of parameter?

but from what i see in git diff view, its been like that for some time now in which case please ignore my comment

I actually have more ideas on how to refactor things here but I don't think it was appropriate to do these changes in this PR (I've deleted code that I spent a couple of hours on with some of these changes I had in mind and decided against pushing further).

This actually ties to the constant question: ideally, the values in OdoSettings would be structs, possibly implementing Validatable that I added not long ago (though, they might need something more like a Convertible interface), each struct having a name and type along with some validation / conversion logic.

AppVeyorBot · 2019-02-01T16:25:45Z

✅ odo build 1347 completed and artifacts can be found here (for commit ad8812ac84 by @metacosm)

AppVeyorBot · 2019-02-01T19:39:28Z

✅ odo build 1351 completed and artifacts can be found here (for commit e0be71991c by @metacosm)

anmolbabu

LGTM...
Works for me

amitkrout · 2019-02-04T09:07:17Z

tests/e2e/e2e_test.go

 		})
 		It("should be checking to see if timeout is the same as the constant", func() {
-			configOutput := runCmd("odo utils config view|grep Timeout")
+			configOutput := runCmd("odo utils config view|grep " + config.TimeoutSetting)


Avoid using terminal specific command like grep. regular expression would be an alternate choice and can be run on any terminal.

I didn't create these tests, they've been like this for a while, not going to change them now.

Ok, will track this change in #1129 then.

amitkrout

Major command odo utils config set ... is missed as part of this pr. Would you please add scenario for odo utils config set ... in integration test too.

$ ./odo utils -h
Utilities for terminal commands and modifying Odo configurations

Usage:
  odo utils [command]

[…]

   # Set a configuration value
   odo utils config set UpdateNotification false
   odo utils config set NamePrefix "app"
   odo utils config set Timeout 20
	
  # For viewing the current configuration
   odo utils config view
  
[…]

surajnarwade

LGTM 🎉

Might be more refactoring opportunities.

metacosm · 2019-02-04T14:14:11Z

Major command odo utils config set ... is missed as part of this pr. Would you please add scenario for odo utils config set ... in integration test too.

$ ./odo utils -h
Utilities for terminal commands and modifying Odo configurations

Usage:
  odo utils [command]

[…]

   # Set a configuration value
   odo utils config set UpdateNotification false
   odo utils config set NamePrefix "app"
   odo utils config set Timeout 20
	
  # For viewing the current configuration
   odo utils config view
  
[…]

Actually, set is covered: https://github.com/redhat-developer/odo/blob/master/tests/e2e/e2e_test.go#L92-L99

amitkrout

LGTM

Actually, set is covered: https://github.com/redhat-developer/odo/blob/master/tests/e2e/e2e_test.go#L92-L99

Cool!!!
My bad, could not recognize it. Thanks for the reference

AppVeyorBot · 2019-02-04T14:49:43Z

✅ odo build 1386 completed and artifacts can be found here (for commit f498b0bb8f by @metacosm)

metacosm requested review from anmolbabu, geoand, mohammedzee1000 and syamgk as code owners January 31, 2019 14:36

metacosm requested a review from surajnarwade as a code owner January 31, 2019 14:59

metacosm requested a review from amitkrout as a code owner January 31, 2019 16:07

metacosm force-pushed the 1194 branch from 6033802 to 363d66e Compare January 31, 2019 16:14

anmolbabu reviewed Feb 1, 2019

View reviewed changes

anmolbabu approved these changes Feb 4, 2019

View reviewed changes

amitkrout reviewed Feb 4, 2019

View reviewed changes

amitkrout suggested changes Feb 4, 2019

View reviewed changes

surajnarwade approved these changes Feb 4, 2019

View reviewed changes

metacosm added 10 commits February 4, 2019 14:31

Initial split of commands, using NewCmd* pattern.

aed11a3

Implement Complete/Validate/Run pattern for terminal and simplify.

60b4df3

Remove unneeded code.

17ce262

Implement Complete/Validate/Run pattern for view.

5252753

Use LongDesc from k8s templates.

3f4b906

Implement Complete/Validate/Run pattern for set.

f4dca2e

Sort shell names.

b99c5e0

Move supported parameters support to pkg/config. Add tests.

7d3f073

Might be more refactoring opportunities.

Add documentation.

7761bbd

Factor out GetSortedKeys function.

452bad8

metacosm added 6 commits February 4, 2019 14:31

Re-structure things to avoid import cycles.

04b678d

Fix format.

daaf7b0

Output supported parameters in alphabetical order.

4865152

Extract constants and use them where possible.

0f0af75

Fix broken e2e test and improved default one.

9c26630

Fix FormatSupportedParameters and long description of config and set.

e883075

metacosm force-pushed the 1194 branch from 8e49b95 to e883075 Compare February 4, 2019 14:18

amitkrout approved these changes Feb 4, 2019

View reviewed changes

Add missing documentation.

f9e6912

metacosm merged commit 12f7c62 into redhat-developer:master Feb 4, 2019

metacosm deleted the 1194 branch February 4, 2019 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor utils command to follow Complete/Validate/Run pattern #1264

Refactor utils command to follow Complete/Validate/Run pattern #1264

metacosm commented Jan 31, 2019

codeclimate bot commented Jan 31, 2019 •

edited

Loading

AppVeyorBot commented Jan 31, 2019

AppVeyorBot commented Jan 31, 2019

codecov bot commented Jan 31, 2019 •

edited

Loading

geoand commented Feb 1, 2019

anmolbabu Feb 1, 2019 •

edited

Loading

metacosm Feb 1, 2019

metacosm Feb 1, 2019

anmolbabu Feb 1, 2019

anmolbabu Feb 1, 2019

metacosm Feb 1, 2019

metacosm Feb 1, 2019

AppVeyorBot commented Feb 1, 2019

AppVeyorBot commented Feb 1, 2019

anmolbabu left a comment

amitkrout Feb 4, 2019

metacosm Feb 4, 2019

amitkrout Feb 4, 2019

amitkrout left a comment •

edited

Loading

surajnarwade left a comment

metacosm commented Feb 4, 2019

amitkrout left a comment

AppVeyorBot commented Feb 4, 2019

Refactor utils command to follow Complete/Validate/Run pattern #1264

Refactor utils command to follow Complete/Validate/Run pattern #1264

Conversation

metacosm commented Jan 31, 2019

Was the change discussed in an issue?

How to test changes?

codeclimate bot commented Jan 31, 2019 • edited Loading

AppVeyorBot commented Jan 31, 2019

AppVeyorBot commented Jan 31, 2019

codecov bot commented Jan 31, 2019 • edited Loading

Codecov Report

geoand commented Feb 1, 2019

anmolbabu Feb 1, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Feb 1, 2019

AppVeyorBot commented Feb 1, 2019

anmolbabu left a 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

amitkrout left a comment • edited Loading

Choose a reason for hiding this comment

surajnarwade left a comment

Choose a reason for hiding this comment

metacosm commented Feb 4, 2019

amitkrout left a comment

Choose a reason for hiding this comment

AppVeyorBot commented Feb 4, 2019

codeclimate bot commented Jan 31, 2019 •

edited

Loading

codecov bot commented Jan 31, 2019 •

edited

Loading

anmolbabu Feb 1, 2019 •

edited

Loading

amitkrout left a comment •

edited

Loading