Bug - Fixed a bug where -version fails due to its dependency on docker client #1118

yhlee-aws · 2017-11-29T22:57:43Z

Summary

Implementation details

Testing

Builds on Linux (make release)
Builds on Windows (go build -out amazon-ecs-agent.exe ./agent)
Unit tests on Linux (make test) pass
Unit tests on Windows (go test -timeout=25s ./agent/...) pass
Integration tests on Linux (make run-integ-tests) pass
Integration tests on Windows (.\scripts\run-integ-tests.ps1) pass
Functional tests on Linux (make run-functional-tests) pass
Functional tests on Windows (.\scripts\run-functional-tests.ps1) pass

New tests cover the changes:

Description for the changelog

Making -version flag for agent independent of docker client, and removing docker version from output.
% docker run --rm --env-file ~/env.txt amazon/amazon-ecs-agent:make -version
Amazon ECS Agent:
Version: 1.16.0
Commit: c2836d6
Dirty: true

Licensing

This contribution is under the terms of the Apache 2.0 License:

nmeyerhans

Our changelog entries should follow the format in CHANGELOG.md, and should be included in that file.

It'd also be good to squash the commits in this PR into a single one before we merge.

jhaynes

Should/can we add any tests around this change? (e.g. make sure we can get the version without a successful Docker API call?)

jhaynes · 2017-11-29T23:43:17Z

agent/version/formatting.go

+	"fmt"
+
+	"github.com/aws/amazon-ecs-agent/agent/sighandlers/exitcodes"
+)

 type Versioner interface {


Is there a reason to leave this interface around?

Versioner interface isn't used anywhere regardless of this change, removing it.

jhaynes · 2017-11-29T23:46:30Z

agent/app/agent.go

@@ -170,12 +167,6 @@ func newAgent(
 	}, nil
 }

-// printVersion prints the ECS Agent version string
-func (agent *ecsAgent) printVersion() int {


It looks like agent/app/agent_integ_test.go calls printVersion. There is also a windows mock that can probably go away here: agent/app/agent_windows_test.go.

For the agent_integ_test, I'll replace the call to printVersion with printECSAttributes, now that printVersion doesn't actually depend on newAgent call.
Removing the mock from agent_window_test.

samuelkarp

I'd prefer not to eliminate the Docker version from the output here, but rather to just skip it when it's unavailable.

aaithal · 2017-11-30T23:19:27Z

@samuelkarp why would prefer to keep docker version being printed with Agent version? Can't you get it from other sources? I'm not sure if we really need to print docker version when printing agent version (I'm trying to think of use-cases which would require it, but can't seem to think of any)

nmeyerhans · 2017-11-30T23:28:51Z

IMO the -version flag should get us the agent version and little, if anything, else.

We could consider adding a -info flag or similar that gives us additional info, similar to docker info. This -info flag could provide some additional information about the agent's runtime environment, including some Docker details.

samuelkarp · 2017-11-30T23:30:36Z

@aaithal I'd like an easy way for us to ask customers to run one command that gives enough information for troubleshooting. @nmeyerhans's suggestion of -info satisfies that, so I'd be okay removing Docker version if we implement something like -info.

aaithal · 2017-11-30T23:33:30Z

makes sense. just noting that we don't usually ask customers to run the -version command in our trouble-shooting guide. let's get rid of Docker version from -version with a plan to add -info in the future.

aaithal

please rewrite the commit history before merging this so that we can get rid of messages like the one in 86d5030. looks good otherwise 🛥

aaithal · 2017-11-30T23:37:22Z

CHANGELOG.md

@@ -1,5 +1,8 @@
 #Changelog

+## Unreleased
+* Bug - Fixed a bug where -version fails due to its dependency on docker client. [#1118](https://github.com/aws/amazon-ecs-agent/pull/1118)


nit: -version would be better

richardpen · 2017-11-30T23:20:36Z

agent/version/formatting.go

@@ -26,7 +26,7 @@ type Versioner interface {
 //        Version: 0.0.3
 //        Commit: 4bdc7fc
 //        DockerVersion: 1.5.0
-func PrintVersion(extra ...Versioner) {
+func PrintVersion() int {


Can you update the comments also.

nmeyerhans · 2017-12-01T00:49:24Z

LGTM. My only request is that the commit message be fleshed out a little bit. https://chris.beams.io/posts/git-commit/ has some good guidelines for how to structure the text.

jhaynes

lgtm with @nmeyerhans' suggestion about the commit message.

yhlee-aws requested review from adnxn, samuelkarp, aaithal, jhaynes and richardpen November 29, 2017 22:57

nmeyerhans suggested changes Nov 29, 2017

View reviewed changes

yhlee-aws changed the title ~~Version api~~ agent, formatting: version flag fix Nov 29, 2017

yhlee-aws changed the title ~~agent, formatting: version flag fix~~ Bug - Fixed a bug where -version fails due to its dependency on docker client Nov 30, 2017

jhaynes reviewed Nov 30, 2017

View reviewed changes

samuelkarp suggested changes Nov 30, 2017

View reviewed changes

nmeyerhans mentioned this pull request Nov 30, 2017

agent -version does not print version #1035

Closed

aaithal approved these changes Nov 30, 2017

View reviewed changes

richardpen reviewed Nov 30, 2017

View reviewed changes

yhlee-aws force-pushed the version-api branch from a08982c to 79c0e50 Compare December 1, 2017 00:14

app,version: removing docker version from -version api

d71caff

yhlee-aws force-pushed the version-api branch from 955ec68 to d71caff Compare December 1, 2017 00:38

richardpen approved these changes Dec 1, 2017

View reviewed changes

jhaynes approved these changes Dec 1, 2017

View reviewed changes

yhlee-aws merged commit 6527d7d into aws:dev Dec 1, 2017

yhlee-aws deleted the version-api branch December 5, 2017 20:53

aaithal mentioned this pull request Dec 18, 2017

V1.17.0 release #1145

Closed

8 tasks

aaithal added this to the 1.17.0 milestone Jan 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug - Fixed a bug where -version fails due to its dependency on docker client #1118

Bug - Fixed a bug where -version fails due to its dependency on docker client #1118

yhlee-aws commented Nov 29, 2017 •

edited

Loading

nmeyerhans left a comment

jhaynes left a comment

jhaynes Nov 29, 2017

yhlee-aws Nov 30, 2017

jhaynes Nov 29, 2017

yhlee-aws Nov 30, 2017

samuelkarp left a comment

aaithal commented Nov 30, 2017 •

edited

Loading

nmeyerhans commented Nov 30, 2017

samuelkarp commented Nov 30, 2017

aaithal commented Nov 30, 2017

aaithal left a comment

aaithal Nov 30, 2017

yhlee-aws Dec 1, 2017

richardpen Nov 30, 2017

yhlee-aws Dec 1, 2017

nmeyerhans commented Dec 1, 2017

jhaynes left a comment

Bug - Fixed a bug where -version fails due to its dependency on docker client #1118

Bug - Fixed a bug where -version fails due to its dependency on docker client #1118

Conversation

yhlee-aws commented Nov 29, 2017 • edited Loading

Summary

Implementation details

Testing

Description for the changelog

Licensing

nmeyerhans left a comment

Choose a reason for hiding this comment

jhaynes 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

samuelkarp left a comment

Choose a reason for hiding this comment

aaithal commented Nov 30, 2017 • edited Loading

nmeyerhans commented Nov 30, 2017

samuelkarp commented Nov 30, 2017

aaithal commented Nov 30, 2017

aaithal 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

nmeyerhans commented Dec 1, 2017

jhaynes left a comment

Choose a reason for hiding this comment

yhlee-aws commented Nov 29, 2017 •

edited

Loading

aaithal commented Nov 30, 2017 •

edited

Loading