Replace swarm script with AMP CLI #455

freignat91 · 2016-11-14T12:10:46Z

related to #437

add amp platform commands: ([-v]: verbose, [-s]:silence, [f]: force

amp platform pull |-v] [-s]
amp platform start [-v] [-s] [-f]
amp platform stop [-v] [-s]
amp platform status [-v] [-s]
amp platform monitor

or using alias pf as:

amp pf start -v
amp pf stop
...

tested on ubuntu and os/x

tests:

make install
stop amp: amp platform stop
remove all images and containers
execute: amp platform pull -v : pull all images needed for amp (-v: to have details)
execute: amp platform monitor in another console to monitor amp
execute: amp platform start -v in the first console
-execute: amp platform status it returns 'running'
execute: make test
execute: amp platform stop

ndegory · 2016-11-14T12:34:47Z

cmd/amp/platform_infrastructure.go

+					Aliases: []string{"amp-agent"},
+				},
+			},
+		}, "nats", "influxdb")


nats is not an upstream dependency of telegraf-agent

ndegory · 2016-11-14T12:41:01Z

cmd/amp/plateform_status.go

@@ -0,0 +1,41 @@
+package main


please rename the file to platform_status.go

ndegory · 2016-11-14T12:46:46Z

cmd/amp/platform_manager.go

+	if s.force {
+		s.printf(colWarn, "Force mode: on\n")
+	}
+	defaultHeaders := map[string]string{"User-Agent": "amplifier-1.0"}


can't we use the version from the binary instead? (at build time there's a VERSION env variable available)

In fact, it should have version here

ndegory · 2016-11-14T12:48:24Z

cmd/amp/platform_manager.go

+	for _, name := range stack.volumesToRemove {
+		s.removeVolume(name)
+	}
+	for {


there's a risk of infinite loop, should't we limit the retries and fail if it wasn't able to start all the services?

ndegory · 2016-11-14T12:51:33Z

cmd/amp/platform_manager.go

+	if err != nil {
+		return err
+	}
+	//Verify fist that it exists at least one container running


s/it exists/there's/

ndegory · 2016-11-14T12:53:07Z

cmd/amp/platform_manager.go

+		}
+	}
+	service.failed = false
+	//No container running and failling then service is still starting without error


s/failling/failing/

ndegory · 2016-11-14T12:57:34Z

cmd/amp/platform_manager.go

+}
+
+func (s *ampManager) forceService(service *ampService) {
+	s.printf(colWarn, "Force mode: service %s is concidered as ready\n", service.name)


s/concidered/considered/
and I think it would be better to say "status of service %s is forced to 'ready'\n"

ndegory · 2016-11-14T12:59:13Z

cmd/amp/platform_manager.go

+		s.docker.VolumeRemove(s.ctx, name, true)
+		ret, err := s.docker.VolumeList(s.ctx, filter)
+		if err == nil {
+			return fmt.Errorf("Impossible to get volume list: %v\n", err)


s/Impossible/Failed/

ndegory · 2016-11-14T13:02:56Z

cmd/amp/platform_manager.go

+		if err != nil {
+			s.printf(colError, "image %s pull error: %v\n", image, err)
+		}
+		data := make([]byte, 1000, 1000)


what happens if the stack is big enough to fill the array?

it's not possible, it's not related to stack but to the reader which adjust data to the array size

ndegory · 2016-11-14T13:08:11Z

cmd/amp/platform_pull.go

+	RootCmd.AddCommand(PlatformPull)
+}
+
+func pullAMPImages(cmd *cobra.Command, args []string) error {


amp pull --server=remote-server:8080 pulls images locally, which doesn't make any sense

to be improved later, --server is a amp persistent flag which have nothing to do with pull for this PR concidering we wanted to have commands separated from amp code and so from amplifier

I disagree, the --server (or the .amp.yaml config) is a configuration for all command of this binary. It should apply to the pull command. If I configure the CLI to manage an infra stack on a remote swarm manager, I don't want it to pull images locally.

yes, i got that, but it's not the target of the PR for now. Added error when --server is used with pull

ndegory · 2016-11-14T13:30:38Z

.travis.yml

-  - ./swarm pull --min
-  - ./swarm start --min
+  - amp pull
+  - appp start


ndegory · 2016-11-14T13:31:14Z

cmd/amp/platform_manager.go

@@ -214,8 +214,8 @@ func (s *ampManager) monitor(stack *ampStack) {
 				cols[2] = len(serv.status) + 2
 			}
 		}
-		fmt.Printf("%s%s%s%s%s\n", col("ID", cols[0]), col("SERVICE", cols[1]), col("STATUS", cols[2]), col("MODE", cols[3]), col("REPLICAS", cols[4]))
-		fmt.Printf("%s%s%s%s%s\n", col("-", cols[0]), col("-", cols[1]), col("-", cols[2]), col("-", cols[3]), col("-", cols[4]))
+		fmt.Printf("%s%s%s%s%s%s\n", col("ID", cols[0]), col("SERVICE", cols[1]), col("STATUS", cols[2]), col("MODE", cols[3]), col("REPLICAS", cols[4]), col("TASK FAILED", cols[5]))


s/TASK FAILED/FAILED TASKS/

ndegory · 2016-11-14T14:07:30Z

.travis.yml

-  - ./swarm start --min
+  - make install
+  - amp pull
+  - app start


what is app? Shouldn't it be amp?

freignat91 force-pushed the cmd-start branch 2 times, most recently from 23842b1 to a178652 Compare November 14, 2016 12:15

ndegory self-assigned this Nov 14, 2016

ndegory suggested changes Nov 14, 2016

View reviewed changes

freignat91 force-pushed the cmd-start branch from 1664b8f to 2f6268f Compare November 14, 2016 13:31

ndegory suggested changes Nov 14, 2016

View reviewed changes

freignat91 force-pushed the cmd-start branch 4 times, most recently from dfc2ebd to 1358adc Compare November 14, 2016 14:07

ndegory reviewed Nov 14, 2016

View reviewed changes

.travis.yml

- ./swarm start --min

- make install

- amp pull

- app start

Copy link

Contributor

ndegory Nov 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is app? Shouldn't it be amp?

freignat91 force-pushed the cmd-start branch 14 times, most recently from d1cf1ce to 86d56ff Compare November 15, 2016 15:20

ndegory approved these changes Nov 15, 2016

View reviewed changes

freignat91 force-pushed the cmd-start branch 3 times, most recently from 126c593 to 8575c37 Compare November 16, 2016 06:41

add amp commands

d0a6529

freignat91 force-pushed the cmd-start branch from c3b6ea9 to d0a6529 Compare November 16, 2016 07:57

generalhenry merged commit 50a1cef into master Nov 16, 2016

generalhenry deleted the cmd-start branch November 16, 2016 18:37

neha-viswanathan mentioned this pull request Nov 17, 2016

amp platform start command fails to execute #472

Closed

ndegory changed the title ~~Cmd start~~ Replace swarm script with AMP CLI Dec 12, 2016

ndegory added the area/cli label Dec 12, 2016

ndegory added this to the 0.4.0 milestone Dec 12, 2016

subfuzion unassigned ndegory Apr 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace swarm script with AMP CLI #455

Replace swarm script with AMP CLI #455

freignat91 commented Nov 14, 2016 •

edited

Loading

ndegory Nov 14, 2016

freignat91 Nov 14, 2016

ndegory Nov 14, 2016

freignat91 Nov 14, 2016

ndegory Nov 14, 2016

freignat91 Nov 14, 2016

ndegory Nov 14, 2016

freignat91 Nov 14, 2016

ndegory Nov 14, 2016

freignat91 Nov 14, 2016

ndegory Nov 14, 2016

freignat91 Nov 14, 2016

ndegory Nov 14, 2016

freignat91 Nov 14, 2016

ndegory Nov 14, 2016

freignat91 Nov 14, 2016

ndegory Nov 14, 2016

freignat91 Nov 14, 2016

ndegory Nov 14, 2016

ndegory Nov 14, 2016

freignat91 Nov 14, 2016 •

edited

Loading

ndegory Nov 14, 2016

freignat91 Nov 14, 2016 •

edited

Loading

ndegory Nov 14, 2016

freignat91 Nov 14, 2016

ndegory Nov 14, 2016

ndegory Nov 14, 2016

Replace swarm script with AMP CLI #455

Replace swarm script with AMP CLI #455

Conversation

freignat91 commented Nov 14, 2016 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freignat91 Nov 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freignat91 Nov 14, 2016 • 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

freignat91 commented Nov 14, 2016 •

edited

Loading

freignat91 Nov 14, 2016 •

edited

Loading

freignat91 Nov 14, 2016 •

edited

Loading