[OCONF-166] coconut conf import command by graduta · Pull Request #71 · AliceO2Group/Control

graduta · 2019-10-01T08:10:52Z

No description provided.

…ub.com:AliceO2Group/Control into feature/OCONF-166-coconut-import-command

…/OCONF-166-coconut-import-command

teo · 2019-10-01T09:09:26Z

Fixes please:

import command

s/Extension of the file should be/Supported file extensions. To force a specific configuration format, see flag --format/-f/
s/Please check through the already existing components:/Available components in configuration database:/
In all simple lists (starting with -), make sure that there's a single space between - and the first letter of the list item.
s/If you wish to add a new component, please use flag -n/-new-component to create a new component/To create a new component, see flag --new-component/-n/
s/A new component has been created/New component created/ s/A new entry has been created/New entry created/ s/The following configuration key has been added/Configuration key created/
s/Component read-teo-test already exists, thus flag -n/-new-component cannot be used/Invalid use of flag --new-component/-n: component red(read-teo-test) already exists/
s/Component read-teo-test has been updated//
s/Entry myEntry2 has been updated/Entry updated: myEntry2/

show command

s/The configuration show command requests by default the latest
configuration for the specified component and entry. It can request exact
time configuration by specifying wanted timestamp as flag/The configuration show command returns the most recent configuration revision for the specified component and entry. It can also return a specific revision, requested with the --timestamp/-t flag./

Suggestions:

Look into empty lines at the end of output and remove them.

teo · 2019-10-01T09:11:42Z

+coconut conf import <component/entry> <file_path> --format=json
+coconut conf import <component/entry> <file_path>.json
+`,
+	Short: "Import a configuration file for the component and entry specified",


Import a configuration file for the specified component and entry

teo · 2019-10-01T09:12:39Z

+	Use:   "import <component> <entry> <file_path>",
+	Aliases: []string{"i", "imp"},
+	Example: `coconut conf import <component> <entry> <file_path>
+coconut conf import <component/entry> <file_path>


Minor nitpick: I'd put <component>/<entry> instead of <component/entry> as component and entry are the variables here, which we compose in one way or another.

teo · 2019-10-01T09:12:52Z

+coconut conf import <component/entry> <file_path>.json
+`,
+	Short: "Import a configuration file for the component and entry specified",
+	Long: `The configuration import command will generate a timestamp and save


s/will generate/generates/

teo · 2019-10-01T09:14:45Z

+`,
+	Short: "Import a configuration file for the component and entry specified",
+	Long: `The configuration import command will generate a timestamp and save
+the configuration file under the component/entry/timestamp path in Consul. Default


... save the configuration file to Consul under the path <component>/<entry>/<timestamp>.

teo · 2019-10-01T09:19:02Z

+	Short: "Import a configuration file for the component and entry specified",
+	Long: `The configuration import command will generate a timestamp and save
+the configuration file under the component/entry/timestamp path in Consul. Default
+accepted file extensions are JSON, YAML, TOML, INI`,


s/Default accepted file extensions are/Supported configuration file types are JSON, YAML, TOML and INI, and their file extensions are recognized automatically./

teo · 2019-10-01T09:19:27Z

+func init() {
+	configurationCmd.AddCommand(configurationImportCmd)
+	configurationImportCmd.Flags().BoolP("new-component", "n",  false, "used to add a new component")
+	configurationImportCmd.Flags().StringP("format", "f",  "", "used to specify a different extension file than default (JSON, YAML, INI, TOML)")


force a specific configuration file type, overriding any file extension

teo · 2019-10-01T09:20:49Z

+
+func init() {
+	configurationCmd.AddCommand(configurationImportCmd)
+	configurationImportCmd.Flags().BoolP("new-component", "n",  false, "used to add a new component")


create a new configuration component while importing entry

teo · 2019-10-01T09:27:46Z

+		for key, _ := range components {
+			componentMsg += "\n-" + key
 		}
+		return errors.New(fmt.Sprintf("Component `" + component + "` does not exist. " +


Let's not use backticks here, either color or nothing. The backticks can potentially make sense in Cobra command doc strings because those are exported as Markdown, but not in error messages.

teo · 2019-10-01T09:32:32Z

+
+func isFileExtensionValid(extension string) bool{
+	extension = strings.ToUpper(extension)
+	return extension == "JSON" || extension == "YAML" || extension == "INI" || extension == "TOML"


Let's also allow .yml just in case. Visible strings should not be changed, just the logic to approve the file extension.

kostorr · 2019-10-01T12:08:34Z

-				err = errors.New(fmt.Sprintf("Requested component name cannot contain character `/` or `@`"))
-				return err, invalidArgs
+			if !isInputSingleValidWord(args[0]) {
+				return  errors.New(fmt.Sprintf(invalidArgsErrMsg)), invalidArgs


This may simply be errors.New(invalidArgsErrMsg). https://golang.org/pkg/errors/#New
fmt.Sprintf() is not necessary if you don't have some fancy arguments you want to handle.

Did not know. Thank you 👍

There's also fmt.Errorf that combines the two for when you do need to handle fancy arguments ;)

kostorr · 2019-10-01T12:16:48Z

-			componentsSet[componentsFullName] = componentTimestamp
-		}
+	entryExists := false
+	if useNewComponent {


Is this clause necessary?

Not anymore. I negated it

kostorr · 2019-10-01T12:19:58Z

+
+	var maxTimeStamp uint64
+	for _, key := range keys {
+		componentTimestamp, err := strconv.ParseUint(strings.TrimPrefix(key, keyPrefix + "/"), 10, 64)


I imagine this is done on purpose here, but this leads to variable shadowing, which could in turn lead to errors. A very good explanation can be found here.

In this case I am not interested in stopping the whole command if one of the timestamps is bad. Thus, I am adding it to the list of elements only if it is a valid one. So even if the timestamp is not correct, it will be ignored.

The error that I am interested here is the one with emptyData

teo · 2019-10-02T10:11:03Z

+coconut conf import <component> <entry> <file_path> --new-component
+coconut conf import <component> <entry> <file_path> --format=json
+coconut conf import <component> <entry> <file_path>.json


We seem to be missing the <component>/<entry> bits.

They are not missing as the requirement was to have exactly 3 arguments:

coconut conf import <component name> <entry name> <file path> [options]

Would you like that option added?

I see, so coconut conf import <component/entry> <file_path>.json never worked even when it was documented?

No. It was a mistake when documentation was added and on second look of the requirements it was then removed.

If we wish to support that we can added so that we are in line with the
coconut conf show command

Yes, that would be splendid, and also the final thing before we merge.

The feature implementation looks good, could you please just add back the relevant documentation lines for the <component>/<entry> syntax? Once this is done we can merge.

teo · 2019-10-02T10:12:08Z

 	nonZero = iota
 	invalidArgs = iota // Provided args by the user are invalid
-	invalidArgsErrMsg = "Component and Entry names cannot contain `/ or  `@`"
+	invalidArgsErrMsg = "component and Entry names cannot contain `/ or  `@`"


s/Entry/entry/

teo · 2019-10-02T10:17:12Z

 	}
 	fullKey :=  red(component) + "/" + blue(entry) + "@" + strconv.FormatInt(timestamp, 10)
-	userMsg += "The following configuration key has been added: " + fullKey
+	userMsg += "Configuration created: " + fullKey


I know we discussed this at length trying to find the best wording and we settled on this, but after additional thought I realized that:

this string is the only one that's always printed (the ones for new entry and component aren't)

this string is the last (and sometimes only) line of output in response to a coconut conf import command.

Therefore I propose the following:
userMsg += "Configuration imported: " + fullKey

graduta added 12 commits August 29, 2019 15:07

[OCONF-166] Add coconut import command

c82029f

[OCONF-166] Refactor

f8879cd

[OCONF-166] Add extension check

d51d762

[OCONF-166] Solve merge conflicts

0b8b111

[OCONF-166] Fix compilation issues

2869928

[OCONF-166] Add coconut history and solve merge conflicts

4b65996

Merge branch 'feature/OCONF-184-coconut-conf-history-command' of gith…

5a1f8cd

…ub.com:AliceO2Group/Control into feature/OCONF-166-coconut-import-command

Merge branch 'master' of github.com:AliceO2Group/Control into feature…

a30d6d9

…/OCONF-166-coconut-import-command

[OCONF-166] Add component & entry check for new entries

3f14f1f

[OCONF-166] Add extension file logic

bc27d15

[OCONF-166] Change error messages

ada0a70

[OCONF-166] Syntax updates

fef336b

graduta added the enhancement label Oct 1, 2019

graduta requested review from kostorr and teo October 1, 2019 08:10

graduta self-assigned this Oct 1, 2019

teo suggested changes Oct 1, 2019

View reviewed changes

kostorr reviewed Oct 1, 2019

View reviewed changes

graduta added 2 commits October 1, 2019 14:58

[OCONF-166] Address comments on strings

dddefe5

[OCONF-166] Fix compilation issue

66b45d3

teo suggested changes Oct 2, 2019

View reviewed changes

graduta added 3 commits October 2, 2019 12:23

[OCONF-166] Update user message

bac7e63

[OCONF-166] Add component/entry format

8cc6139

[OCONF-166] Remove commented lines

ee87a65

teo approved these changes Oct 3, 2019

View reviewed changes

[OCONF-166] Update documentation

cb43b7f

teo approved these changes Oct 3, 2019

View reviewed changes

graduta merged commit e76aacd into master Oct 3, 2019

graduta deleted the feature/OCONF-166-coconut-conf-import-command branch October 3, 2019 10:16

Conversation

graduta commented Oct 1, 2019

Uh oh!

teo commented Oct 1, 2019

import command

show command

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

graduta Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

graduta Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

graduta Oct 1, 2019 •

edited

Loading

graduta Oct 2, 2019 •

edited

Loading