Show host the task is running on in emp ps
#983
Conversation
| @@ -58,6 +58,9 @@ func listDynos(w io.Writer) { | |||
| func listDyno(w io.Writer, d *heroku.Dyno) { | |||
| listRec(w, | |||
| d.Name, | |||
| d.Id, | |||
| d.ContainerInstanceID, | |||
There was a problem hiding this comment.
In general, the container instance id isn't very useful, and it's easy to correlate in AWS if we have the EC2 instance id. I think we should leave this off for the first version.
I think if we just start with showing the ec2 instance id, then it won't add too many bytes and we can just show it. |
|
Other than those comments, this is awesome. This will be really useful! |
Nosajool
force-pushed
the
jl-show-task-host
branch
from
83e166d
to
14fb60c
Compare
They have the same implementation From the comment: #983 (comment)
| return nil, err | ||
| } | ||
| for _, f := range resp.Failures { | ||
| return nil, fmt.Errorf("error describing container instance Arn=%s err=%s", aws.StringValue(f.Arn), aws.StringValue(f.Reason)) |
There was a problem hiding this comment.
Minor nit, can we make this error:
fmt.Errorf("error describing container instance %s: %s", aws.StringValue(f.Arn), aws.StringValue(f.Reason))
|
Just a couple small nits, but overall this looks good to go. Just needs a rebase since #985 was merged. |
Closes #976 Add mocks for cloudformation tests fix cli tests
Also make container instance request based on cluster Fix cloudformation tests Fix cli tests
Also update CHANGELOG.md
| @@ -24,6 +24,7 @@ | |||
| * The Scheduler now has a `Restart` method which will trigger a restart of all the processes within an app. Previously, a "Restart" just re-released the app. Now schedulers like the cloudformation backend can optimize how the restart is handled. [#697](https://github.com/remind101/empire/issues/697) | |||
| * `emp run` now publishes an event when it is ran. [#954](https://github.com/remind101/empire/pull/954) | |||
| * `emp rollback` requires confirmation if rolling back more than 9 versions. [#975](https://github.com/remind101/empire/pull/975) | |||
| * `emp ps` now displays the task's host. [#983](https://github.com/remind101/empire/pull/983) | |||
There was a problem hiding this comment.
Oops, this should get added to ## HEAD since 0.11.0 was already released.
Also update the Fake scheduler to return instances with fake host ids.
Nosajool
force-pushed
the
jl-show-task-host
branch
from
14fb60c
to
b6bcf7e
Compare
Closes #976
Opening this since I'd like some feedback, however I don't think this is the right approach since it requires a request to
DescribeContainerInstancesfor each process which will quickly rate limit us. Looking into if it is possible to just make one batch request for all of the containers. If not, might need to cache like in #902.Also, the output is getting pretty wide so I was wondering if maybe this stuff should be only shown if you pass the
-vflag toemp ps. Then we could add other information here like @sanjayprabhu's "commit message" request. https://remind.slack.com/archives/empire/p1471910137001348New output
v2.web.1a1a11a1-aaa1-1aaa-a11a-aaa111a1aa1a i-aa111aa1 1X RUNNING 89m "acme-inc server"