Added Index numbers for compose files to be used in "compose list/set" #214

cmbahadir · 2019-08-15T09:05:36Z

Just wanted to contribute with below concerns;

Description and name of the argument (chosen as "index").
Location of the tests
And also i am new to go world. :)

Fixes: #199

Signed-off-by: Cihan Mete Bahadir c.mete.bahadir@gmail.com

.gitignore

test/init_local_test.go

smacker · 2019-08-19T08:53:26Z

cmd/sourced/cmd/compose.go

@@ -66,18 +68,28 @@ type composeSetDefaultCmd struct {
 	Command `name:"set" short-description:"Set the active docker compose file" long-description:"Set the active docker compose file"`

 	Args struct {
-		Version string `positional-arg-name:"version" description:"Either a revision (tag, full sha1) or a URL to a docker-compose.yml file"`
+		Index string `positional-arg-name:"index" description:"Index of the docker compose file returned from 'sourced compose list'"`


maybe it makes sense to accept both the index and the name?
I see why index can be better in many cases but at the same time, it's easier to switch to master just by typing sourced compose set master instead of listing the files and looking for the correct number.
what do you think?

Do you mean only for master or all rev and URL?

I think that it should work as now + with index. So essentially the tests that are already in master should pass without any change, then it should also work with indexes.

I guess that it's safe to try parsing whether it's an integer first and in case of a parsing error fallback to the current mechanism.

What if there is a branch, or tag called 1?

That's definitely a case that I thought, but it's really and edge case that IMHO could be ignored. I don't think that it makes much sense to have a branch named \d+. But if you prefer you may want to default as integer and provide a flag --rev to force it. But again IMHO it's unnecessary.

We control tags and branches so there is no need to add additional logic for the edge case.

What git does, for example when performing a checkout to a revision that can be resolved to different commits, is to rise an error, and ask for a more precise definition of the revision.

Changed it to both accept index and name by ignoring the edge cases.

dpordomingo · 2019-08-19T10:49:58Z

cmd/sourced/cmd/compose.go

+		active := files[index]
+		err = composefile.SetActive(active)
+		fmt.Println("Active docker compose file was changed successfully.")
+		fmt.Println("To update your current installation use `sourced restart`")


What's happening if the user inits with one config using services a,b,c changes to a new compose defining services c,d, and then the user restarts?
Isn't it error prone because the potential mismatch between old and new config? Or can we assume it's stable enough for oficial versions?
Should that problem be handled by the user depending on initial and final compose configs?

Is it enough to check SetActive error?

This is more an open question for @src-d/applications, to consider if we want to handle that edgecase, or if it should be handled by the user running prune before changing the active compose.

I think this is something to be discussed in the DD about sourced upgrade. IMO if the changes in the services occurred because of an upgrade of sourced (so something expected), then it should be handled by the upgrade process, or in case that it's not possible, it should be correctly notified to the user. Otherwise, if the change occurred because the user is using a custom compose file, then he/she is an advanced user and I would assume that he/she knows what to do.

cmd/sourced/cmd/compose.go

…te Bahadir <c.mete.bahadir@gmail.com> Added tests for compose list/set commands. Signed-off-by: Cihan Mete Bahadir <c.mete.bahadir@gmail.com> Signed-off-by: cmbahadir <c.mete.bahadir@gmail.com>

…an Mete Bahadir <c.mete.bahadir@example.com> Signed-off-by: cmbahadir <c.mete.bahadir@gmail.com>

… <c.mete.bahadir@example.com> Signed-off-by: cmbahadir <c.mete.bahadir@gmail.com>

…ahadir@gmail.com> Signed-off-by: cmbahadir <c.mete.bahadir@gmail.com>

…ahadir <c.mete.bahadir@gmail.com> Signed-off-by: cmbahadir <c.mete.bahadir@gmail.com>

cmbahadir

Added the error check for SetActive
Modified tests to run properly on windows.
changed the .gitignore to not include .vscode

cmd/sourced/cmd/compose.go

smacker · 2019-08-21T08:58:17Z

Thank you @cmbahadir, great work!
Do you think you can implement a fallback described here: https://github.com/src-d/sourced-ce/pull/214/files#r315101967 ?

I think you can ignore the edge case with a number as a tag or a branch for now, we can improve it later.

cmbahadir · 2019-08-21T14:50:02Z

Thank you @cmbahadir, great work!
Do you think you can implement a fallback described here: https://github.com/src-d/sourced-ce/pull/214/files#r315101967 ?

I think you can ignore the edge case with a number as a tag or a branch for now, we can improve it later.

Hi @smacker yes i can implement it but i may need an additional package like ‘reflect’ is it ok to use it?

smacker · 2019-08-21T15:54:53Z

Please help me understand why do you need reflect?

Currently you have

index, err := strconv.Atoi(c.Args.Index)
if err != nil {
  fmt.Println("Provide the index of the docker compose file in 'sourced compose list'")
}

The idea is to try to go old code-path aka composefile.SetActive(c.Args.Index) in such case instead of showing the error. (argument's help text needs to be updated too)

cmbahadir · 2019-08-21T16:50:17Z

Yes @smacker you are right just thought that it may be safer to check the type of the index. I will implement it by handling fallback from strconv.Atoi ;)

…. Signed-off-by: Cihan Mete Bahadir <c.mete.bahadir@gmail.com> Signed-off-by: cmbahadir <c.mete.bahadir@gmail.com>

carlosms · 2019-08-22T14:25:28Z

cmd/sourced/cmd/compose.go

+			active := files[index]
+			err = composefile.SetActive(active)
+		} else {
+			fmt.Println("Index is out of range of the files in 'sourced compose list'")


Maybe instead of printing the message and return nil, this should be
return fmt.Errorf("Index is out of range of the files in 'sourced compose list'")

Since the argument is not valid, a non-0 exit makes sense.

It makes sense to me too.

Thanks for the reviews, i will apply the fixes.

se7entyse7en · 2019-08-23T10:23:11Z

cmd/sourced/cmd/compose.go

+			active := files[index]
+			err = composefile.SetActive(active)
+		} else {
+			fmt.Println("Index is out of range of the files in 'sourced compose list'")


It makes sense to me too.

cmd/sourced/cmd/compose.go

se7entyse7en · 2019-08-23T10:25:32Z

BTW thanks for your contribution @cmbahadir, we really appreciate it! Gonna approve after the last nit-picks 👍.

… return. Signed-off-by: Cihan Mete Bahadir <c.mete.bahadir@gmail.com> Signed-off-by: cmbahadir <c.mete.bahadir@gmail.com>

cmbahadir · 2019-08-23T12:00:23Z

All changes implemented.

cmbahadir force-pushed the singlePullRequest branch from 6368ee0 to 0d738cd Compare August 15, 2019 09:06

smacker reviewed Aug 19, 2019

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

smacker reviewed Aug 19, 2019

View reviewed changes

test/init_local_test.go Outdated Show resolved Hide resolved

smacker reviewed Aug 19, 2019

View reviewed changes

dpordomingo reviewed Aug 19, 2019

View reviewed changes

cmd/sourced/cmd/compose.go Outdated Show resolved Hide resolved

cmbahadir force-pushed the singlePullRequest branch 2 times, most recently from 8cc3e56 to 3ae3561 Compare August 19, 2019 18:59

cmbahadir added 4 commits August 19, 2019 22:00

Added index numbers for docker compose files. Signed-off-by: Cihan Me…

6fd3826

…te Bahadir <c.mete.bahadir@gmail.com> Added tests for compose list/set commands. Signed-off-by: Cihan Mete Bahadir <c.mete.bahadir@gmail.com> Signed-off-by: cmbahadir <c.mete.bahadir@gmail.com>

Removed description for previous version argument. Signed-off-by: Cih…

e6fa4c5

…an Mete Bahadir <c.mete.bahadir@example.com> Signed-off-by: cmbahadir <c.mete.bahadir@gmail.com>

Fixed tests to not fail on windows. Signed-off-by: Cihan Mete Bahadir…

e2eb4b3

… <c.mete.bahadir@example.com> Signed-off-by: cmbahadir <c.mete.bahadir@gmail.com>

Handling SetActive error. Signed-off-by: Cihan Mete Bahadir <c.mete.b…

3473633

…ahadir@gmail.com> Signed-off-by: cmbahadir <c.mete.bahadir@gmail.com>

cmbahadir force-pushed the singlePullRequest branch from 3ae3561 to 3473633 Compare August 19, 2019 19:00

Removed .vscode settings from .gitignore. Signed-off-by: Cihan Mete B…

a8a51d9

…ahadir <c.mete.bahadir@gmail.com> Signed-off-by: cmbahadir <c.mete.bahadir@gmail.com>

cmbahadir force-pushed the singlePullRequest branch from 1a2ea1f to a8a51d9 Compare August 19, 2019 19:12

cmbahadir commented Aug 21, 2019

View reviewed changes

cmd/sourced/cmd/compose.go Outdated Show resolved Hide resolved

Changed compose set to accept both index and name of the compose file…

d39a8d1

…. Signed-off-by: Cihan Mete Bahadir <c.mete.bahadir@gmail.com> Signed-off-by: cmbahadir <c.mete.bahadir@gmail.com>

cmbahadir force-pushed the singlePullRequest branch from 6979870 to d39a8d1 Compare August 21, 2019 17:53

smacker approved these changes Aug 22, 2019

View reviewed changes

carlosms reviewed Aug 22, 2019

View reviewed changes

se7entyse7en suggested changes Aug 23, 2019

View reviewed changes

Removed not hit error fallback and index error transformed into error…

e8ab806

… return. Signed-off-by: Cihan Mete Bahadir <c.mete.bahadir@gmail.com> Signed-off-by: cmbahadir <c.mete.bahadir@gmail.com>

carlosms approved these changes Aug 26, 2019

View reviewed changes

se7entyse7en approved these changes Aug 28, 2019

View reviewed changes

smacker merged commit 4de8f56 into src-d:master Aug 28, 2019

Added Index numbers for compose files to be used in "compose list/set" #214

Added Index numbers for compose files to be used in "compose list/set" #214

Uh oh!

Conversation

cmbahadir commented Aug 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

cmbahadir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smacker commented Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmbahadir commented Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smacker commented Aug 21, 2019

Uh oh!

cmbahadir commented Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Uh oh!

se7entyse7en commented Aug 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmbahadir commented Aug 23, 2019

Uh oh!

Uh oh!

cmbahadir commented Aug 15, 2019 •

edited

Loading

smacker commented Aug 21, 2019 •

edited

Loading

cmbahadir commented Aug 21, 2019 •

edited

Loading

cmbahadir commented Aug 21, 2019 •

edited

Loading

se7entyse7en commented Aug 23, 2019 •

edited

Loading