add config command for toggling update checker #589

syamgk · 2018-07-18T23:36:35Z

add config command
create a new field 'config' in odoconfig file
for storing odo specific configurations

surajnarwade · 2018-07-23T11:43:04Z

@syamgk , there's a problem

✔ ~/go/src/github.com/redhat-developer/odo [pr_589 L|⚑ 29] 
17:12 $ odo config set UpdateNotification false
✔ ~/go/src/github.com/redhat-developer/odo [pr_589 L|⚑ 29] 
17:12 $ odo config get
{UpdateNotification:true}

syamgk · 2018-07-24T12:31:58Z

good catch @surajnarwade
the yaml.unmashal is not working as I thought
am on this, finding a way to make it 'true' in case not specified on odo file

kadel · 2018-07-19T07:31:23Z

cmd/config.go

+var configurationCmd = &cobra.Command{
+	Use:   "config",
+	Short: "Modifying configuration settings",
+	Long:  `Performs edits on configuration settings on config file.`,


"Change configuration values" or something like that might be better.
Regular ODO user doesn't have to know anything about config file.

kadel · 2018-07-19T07:36:19Z

pkg/config/config.go

 	configFileName = "odo"
 )

+// ConfigurationInfo holds all odo specific configurations
+type ConfigurationInfo struct {


This might be a bit confusing name. We already have ConfigInfo that is root struct for the whole odo config handling.

kadel · 2018-07-24T14:31:32Z

pkg/config/config.go

@@ -16,6 +16,11 @@ const (
 	configFileName = "odo"
 )

+// ConfigurationInfo holds all odo specific configurations
+type ConfigurationInfo struct {
+	UpdateNotification bool `json:"updatenotification"`


Make this pointer to bool to make it possible to detect whatever it was set or not.

kadel · 2018-07-24T14:32:17Z

pkg/config/config.go

@@ -112,6 +120,46 @@ func (c *ConfigInfo) writeToFile() error {
 	return nil
 }

+// SetConfiguration Set Odo configurations
+func SetConfiguration(component string, value bool) error {


what is component it is not used inside this function

kadel · 2018-07-24T14:34:09Z

pkg/config/config.go

+}
+
+// GetupdateNotificationStatus returns the value of UpdateNotification config
+func GetupdateNotificationStatus() bool {


What is the difference between this and GetConfiguration?
It looks like they are both doing almost the same.

kadel · 2018-07-24T14:35:16Z

pkg/config/config.go

+
+// Getconfiguration get odo configurations
+func GetConfiguration() error {
+	cfg, err := New()


You shouldn't call New() here. Check other function in this file, and try to follow the same pattern in your functions.

kadel · 2018-07-24T14:39:01Z

cmd/config.go

+		}
+	}, Run: func(cmd *cobra.Command, args []string) {
+
+		switch args[1] {


you can use strconv.ParseBool to avoid this switch case and make the code more robust.

cdrage

Just a few comments

cdrage · 2018-07-26T18:07:00Z

cmd/config.go

+var configurationCmd = &cobra.Command{
+	Use:   "config",
+	Short: "Modifies configuration settings",
+	Long:  `Modifies odo specific configuration settings on config file.`,


Maybe: Modifies Odo specific configuration settings within the config file?

cdrage · 2018-07-26T18:08:40Z

cmd/config.go

+	// 'odo config' is the same as 'odo config get'
+	Args: func(cmd *cobra.Command, args []string) error {
+		if len(args) >= 1 && (args[0] != "get" || args[0] != "set") {
+			return fmt.Errorf("Unknown usage")


Unknown usage? Maybe Unknown command, use set or get

cdrage · 2018-07-26T18:09:11Z

cmd/config.go

+var configurationSetCmd = &cobra.Command{
+	Use:   "set",
+	Short: "Set a value in odo config file",
+	Long:  "Set an individual value in odo configuration file",


in the Odo configuration file

cdrage · 2018-07-26T18:10:07Z

cmd/config.go

+ 	`,
+	Args: func(cmd *cobra.Command, args []string) error {
+		if len(args) < 2 {
+			return fmt.Errorf("Please provide parameter name and value")


Please provide a

cdrage · 2018-07-26T18:10:17Z

cmd/config.go

+		if len(args) < 2 {
+			return fmt.Errorf("Please provide parameter name and value")
+		} else if len(args) > 2 {
+			return fmt.Errorf("Only one value per parameter name is allowed")


remove name

cdrage · 2018-07-26T18:21:42Z

pkg/config/config.go

@@ -112,6 +120,46 @@ func (c *ConfigInfo) writeToFile() error {
 	return nil
 }

+// SetConfiguration Set Odo configurations


could use a better description

cdrage · 2018-07-26T18:21:59Z

pkg/config/config.go

+	cfg.OdoConfigurations.UpdateNotification = value
+	err = cfg.writeToFile()
+	if err != nil {
+		return errors.Wrapf(err, "unable to set updatenotification to %t ", value)


add spacing between update notification

cdrage · 2018-07-26T18:22:23Z

pkg/config/config.go

+func SetConfiguration(component string, value bool) error {
+	cfg, err := New()
+	if err != nil {
+		return errors.Wrap(err, "unable to modify configuration")


maybe unable to modify configuration file ? this modifies an actual file, right?

cdrage · 2018-07-26T18:22:39Z

pkg/config/config.go

+	return nil
+}
+
+// Getconfiguration get odo configurations


More descriptive configuration :)

mik-dass

Accepts any wrong value and updates the UpdateNotification's value

mik-dass · 2018-08-07T12:18:02Z

cmd/config.go

+		if err != nil {
+			fmt.Println(err, "unable to set configuration")
+		}
+		cfg.SetConfiguration(value)


If there are two values that can be updated then there is no check for that. Also if there is a spelling mistake or any other value, then it updates the UpdateNotification value only.

[mdas@localhost odo]$ ./odo config set dajsdion true [mdas@localhost odo]$

The above shouldn't update UpdateNotification's value

kadel · 2018-08-20T09:44:05Z

cmd/config.go

+		configurationGetCmd.Example,
+		configurationSetCmd.Example),
+	Aliases: []string{"configuration"},
+	// 'odo config' is the same as 'odo config get'


odo utils config ;-)

kadel · 2018-08-21T08:53:15Z

cmd/config.go

+}
+
+var configurationGetCmd = &cobra.Command{
+	Use:   "get",


Wouldn't it be better to name this command view? as it lists all the values in config.
I would image that get will allow me to get a value of the configuration option that I specify like odo utils config get updatenotification.

We don't have to implement this right now. But I would prefer renaming this command to view to better match its behavior. WDYT?

kadel · 2018-08-21T08:53:29Z

cmd/config.go

+
+var configurationGetCmd = &cobra.Command{
+	Use:   "get",
+	Short: "View current value",


View current values. might be better

kadel · 2018-08-21T08:54:41Z

cmd/utils.go

+	configurationCmd.AddCommand(configurationSetCmd)
+
+	// Add a defined annotation in order to appear in the help menu
+	//configurationCmd.Annotations = map[string]string{"command": "utility"}


we can delete those comments

kadel · 2018-08-21T16:15:33Z

cmd/config.go

@@ -68,16 +68,16 @@ var configurationSetCmd = &cobra.Command{
 }

 var configurationGetCmd = &cobra.Command{


variable should be also renamed

surajnarwade · 2018-08-23T03:54:16Z

Check minishift config

$ minishift config
Modifies Minishift configuration properties. Some of the configuration properties are equivalent
to the options that you set when you run the 'minishift start' command.

Configurable properties (enter as SUBCOMMAND): 

 * iso-url
 * cpus
 * memory
..........
...........
 * hyperv-virtual-switch

Usage:
  minishift config SUBCOMMAND [flags]
  minishift config [command]

Available Commands:
  get         Gets the value of a configuration property from the Minishift configuration file.
  set         Sets the value of a configuration property in the Minishift configuration file.
  unset       Clears the value of a configuration property in the Minishift configuration file.
  view        Display the properties and values of the Minishift configuration file.

Flags:
  -h, --help   help for config

Global Flags:
      --alsologtostderr                  log to standard error as well as files
      --log_backtrace_at traceLocation   when logging hits line file:N, emit a stack trace (default :0)
      --log_dir string                   If non-empty, write log files in this directory
      --logtostderr                      log to standard error instead of files
      --profile string                   Profile name (default "minishift")
      --show-libmachine-logs             Show logs from libmachine.
      --stderrthreshold severity         logs at or above this threshold go to stderr (default 2)
  -v, --v Level                          log level for V logs. Level varies from 1 to 5 (default 1).
      --vmodule moduleSpec               comma-separated list of pattern=N settings for file-filtered logging

Use "minishift config [command] --help" for more information about a command.

I think we should follow this for odo utls config, as of now, odo utils config and odo utils config view are the same

✔ ~/go/src/github.com/redhat-developer/odo [pr_589 L|⚑ 33] 
09:04 $ odo utils config
PARAMETER             CURRENTVALUE
UpdateNotification    true
✔ ~/go/src/github.com/redhat-developer/odo [pr_589 L|⚑ 33] 
09:23 $ odo utils config view
PARAMETER             CURRENTVALUE
UpdateNotification    true

kadel · 2018-08-24T10:05:41Z

@surajnarwade Agreed odo utils config should just show help message

@syamgk can you please rebase this and

kadel

odo utils config is not a blocker we can fix it in another PR.

It is up to you @surajnarwade ;-)

syamgk · 2018-08-24T11:50:52Z

@surajnarwade changed odo utils config to print the help message

@kadel rebased the PR

If everything looks good I can squash the commit and update the commit message

cdrage

There's no e2e tests for this :(

Since this is a new command, it's suggested that we add one even if it's super simple.

Have a look at my comments however!

cdrage · 2018-08-27T14:05:37Z

cmd/config.go

+   # Available Parameters:
+     UpdateNotification - Controls if an update notification is shown or not (true or false)`,
+	Example: `  # Set a configuration value
+    odo utils config set UpdateNotification false`,


you need to add a newline:

example:

odo utils config set UpdateNotification false `,

cdrage · 2018-08-27T14:07:09Z

cmd/config.go

+   # Available Parameters:
+     UpdateNotification - Controls if an update notification is shown or not (true or false)`,
+	Example: `  # Set a configuration value
+    odo utils config set UpdateNotification false`,


something weird is happening here too. it it 3-spaces not 1-tab, as it is not appearing aligned:

github.com/redhat-developer/odo pr_589 ✔ 3d ⍉ ▶ ./odo utils Utilities for completion, terminal commands and modifying Odo configurations Usage: odo utils [command] Examples: # Bash autocompletion support source <(odo utils completion bash) # Zsh autocompletion support source <(odo utils completion zsh) # Bash terminal PS1 support source <(odo utils terminal bash) # Zsh terminal PS1 support source <(odo utils terminal zsh) # Set a configuration value odo utils config set UpdateNotification false # For viewing the current configuration odo utils config view

cdrage · 2018-08-27T14:07:32Z

cmd/config.go

+
+var configurationViewCmd = &cobra.Command{
+	Use:   "view",
+	Short: "View current values",


View current configuration values

cdrage · 2018-08-27T14:07:36Z

cmd/config.go

+var configurationViewCmd = &cobra.Command{
+	Use:   "view",
+	Short: "View current values",
+	Long:  "View current configuration",


View current configuration values

cdrage · 2018-08-27T14:08:05Z

cmd/config.go

+	Short: "View current values",
+	Long:  "View current configuration",
+	Example: `  # For viewing the current configuration
+    odo utils config view`,


need to add a newline (see above). also change the two tabs to 3-spaces instead (it's messing up alignment when using odo utils --help

cdrage · 2018-08-27T14:09:52Z

cmd/config.go

+	Use:   "config",
+	Short: "Modifies configuration settings",
+	Long: `Modifies Odo specific configuration settings within the config file.
+   # Available Parameters:


This looks odd when doing --help

github.com/redhat-developer/odo pr_589 ✔ 3d ⍉ ▶ ./odo utils config --help Modifies Odo specific configuration settings within the config file. # Available Parameters: UpdateNotification - Controls if an update notification is shown or not (true or false) Usage: odo utils config [flags] odo utils config [command] Aliases: config, configuration Examples: # For viewing the current configuration odo utils config view # Set a configuration value odo utils config set UpdateNotification false Available Commands: set Set a value in odo config file view View current values Flags: -h, --help help for config Global Flags: --alsologtostderr log to standard error as well as files --log_backtrace_at traceLocation when logging hits line file:N, emit a stack trace (default :0) --log_dir string If non-empty, write log files in this directory --logtostderr log to standard error instead of files (default false) --skip-connection-check Skip cluster check --stderrthreshold severity logs at or above this threshold go to stderr (default 2) -v, --v Level log level for V logs --vmodule moduleSpec comma-separated list of pattern=N settings for file-filtered logging Use "odo utils config [command] --help" for more information about a command.

you should instead remove the # and remove the tabbing. Also add a newline between the 1st sentence and Available parameters.

syamgk · 2018-08-28T12:22:04Z

@cdrage added a simple test it passed on CI yay!
https://travis-ci.org/redhat-developer/odo/jobs/421530938#L676

mik-dass · 2018-08-29T14:03:08Z

cmd/config.go

+			fmt.Println(err, ": unable to view configuration")
+		}
+		w := tabwriter.NewWriter(os.Stdout, 5, 2, 2, ' ', tabwriter.TabIndent)
+		fmt.Fprintln(w, "PARAMETER", "\t", "CURRENTVALUE")


@syamgk Please add a _ like CURRENT_VALUE for better readability

surajnarwade

LGTM

mik-dass

LGTM

syamgk · 2018-08-30T13:05:51Z

squashed! now ready for merge :)

- add config command - create a new field 'config' in odoconfig file for storing odo specific configurations

cdrage · 2018-09-04T16:43:51Z

LGTM!

syamgk added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. labels Jul 18, 2018

syamgk mentioned this pull request Jul 19, 2018

Improve update checker #122

Closed

syamgk force-pushed the odo-config branch 5 times, most recently from 928fe24 to ce89cb3 Compare July 23, 2018 06:32

syamgk removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. Required by Prow. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. labels Jul 23, 2018

syamgk force-pushed the odo-config branch 2 times, most recently from 58e04d7 to 37228d3 Compare July 23, 2018 07:20

syamgk changed the title ~~add config command~~ add config command for toggling update checker Jul 23, 2018

kadel reviewed Jul 24, 2018

View reviewed changes

cdrage suggested changes Jul 26, 2018

View reviewed changes

syamgk force-pushed the odo-config branch 4 times, most recently from 9b40e33 to 33dcd32 Compare July 31, 2018 18:37

syamgk mentioned this pull request Aug 5, 2018

make timeout time configurable #617

Closed

syamgk force-pushed the odo-config branch 2 times, most recently from 096374f to d4df9d5 Compare August 6, 2018 11:44

mik-dass suggested changes Aug 7, 2018

View reviewed changes

kadel reviewed Aug 20, 2018

View reviewed changes

kadel reviewed Aug 21, 2018

View reviewed changes

syamgk force-pushed the odo-config branch from c48daa2 to 348a26b Compare August 23, 2018 03:17

surajnarwade added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Aug 24, 2018

kadel approved these changes Aug 24, 2018

View reviewed changes

syamgk force-pushed the odo-config branch from 348a26b to b0f9b8e Compare August 24, 2018 11:26

syamgk removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Aug 24, 2018

syamgk force-pushed the odo-config branch from 39a802e to c3b174a Compare August 27, 2018 05:56

cdrage suggested changes Aug 27, 2018

View reviewed changes

mik-dass reviewed Aug 29, 2018

View reviewed changes

surajnarwade approved these changes Aug 30, 2018

View reviewed changes

mik-dass reviewed Aug 30, 2018

View reviewed changes

syamgk force-pushed the odo-config branch 2 times, most recently from 2504238 to 8506be8 Compare August 30, 2018 13:05

mik-dass approved these changes Aug 30, 2018

View reviewed changes

syamgk added 2 commits August 31, 2018 13:41

add odo utils config command for enable/disable parameters

f35a4d1

- add config command - create a new field 'config' in odoconfig file for storing odo specific configurations

update CLI structure in Readme.md

566445e

syamgk force-pushed the odo-config branch from 8506be8 to 566445e Compare August 31, 2018 08:14

cdrage approved these changes Sep 4, 2018

View reviewed changes

cdrage merged commit 2bb2dca into redhat-developer:master Sep 4, 2018

cdrage mentioned this pull request Sep 5, 2018

confusing installation instruction in new version notification #183

Closed

		@@ -68,16 +68,16 @@ var configurationSetCmd = &cobra.Command{
		}

		var configurationGetCmd = &cobra.Command{

add config command for toggling update checker #589

add config command for toggling update checker #589

Conversation

syamgk commented Jul 18, 2018

surajnarwade commented Jul 23, 2018

syamgk commented Jul 24, 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

Choose a reason for hiding this comment

cdrage 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

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

mik-dass 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

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

surajnarwade commented Aug 23, 2018

kadel commented Aug 24, 2018

kadel left a comment

Choose a reason for hiding this comment

syamgk commented Aug 24, 2018

cdrage 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syamgk commented Aug 28, 2018

mik-dass Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

surajnarwade left a comment

Choose a reason for hiding this comment

mik-dass left a comment

Choose a reason for hiding this comment

syamgk commented Aug 30, 2018

cdrage commented Sep 4, 2018

mik-dass Aug 29, 2018 •

edited

Loading