[wip] Metadata task definition fields #1122
Conversation
| dockerContainerMetadata: dockerMD, | ||
| containerInstanceARN: manager.containerInstanceARN, | ||
| metadataStatus: MetadataReady, | ||
| } | ||
| } | ||
|
|
||
| func parseTaskDefinitionRevision(task *api.Task, containerName string) int { |
There was a problem hiding this comment.
This needs a comment describing what it does. It is also probably worth mentioning that when strconv.Atoi fails with an error, taskDefinitionRevision will be set to the int 0.
agent/containermetadata/types.go
Outdated
| @@ -151,6 +155,8 @@ func (m Metadata) MarshalJSON() ([]byte, error) { | |||
| Cluster: m.cluster, | |||
| ContainerInstanceARN: m.containerInstanceARN, | |||
| TaskARN: m.taskMetadata.taskARN, | |||
| TaskDefinitionFamily: m.taskMetadata.taskDefinitionFamily, | |||
| TaskDefinitionRevision: m.taskMetadata.taskDefinitionRevision, | |||
There was a problem hiding this comment.
nit: the spacing for all the other elements in the map should match this line.
The task struct already has this information, so it's just a matter of plumbing it through. Note that the type of the revision field in the struct is a string, and the type in our CLI is an integer (https://docs.aws.amazon.com/cli/latest/reference/ecs/describe-task-definition.html). This change is consistent with the CLI. (cherry picked from commit fdf1810) Signed-off-by: Josh Oberwetter <oberj@amazon.com>
jtoberon
force-pushed
the
metadata-task-definition-fields
branch
from
8a139f2 to
26d09da
Compare
| return Metadata{ | ||
| cluster: manager.cluster, | ||
| taskMetadata: TaskMetadata{containerName: containerName, taskARN: taskARN}, | ||
| taskMetadata: TaskMetadata{containerName: containerName, taskARN: task.Arn, taskDefinitionFamily: task.Family, taskDefinitionRevision: taskDefinitionRevision}, |
There was a problem hiding this comment.
minor: can you split these into multiple lines?
There was a problem hiding this comment.
Yeah. I couldn't figure out what the line length convention for this repo is. I'll do something reasonable.
| return Metadata{ | ||
| cluster: manager.cluster, | ||
| taskMetadata: TaskMetadata{containerName: containerName, taskARN: taskARN}, | ||
| taskMetadata: TaskMetadata{containerName: containerName, taskARN: task.Arn, taskDefinitionFamily: task.Family, taskDefinitionRevision: taskDefinitionRevision}, |
There was a problem hiding this comment.
same here. please split these into multiple lines (helps with readability)
| ContainerInstanceARN string `json:"ContainerInstanceARN,omitempty"` | ||
| TaskARN string `json:"TaskARN,omitempty"` | ||
| TaskDefinitionFamily string `json:"TaskDefinitionFamily,omitempty"` | ||
| TaskDefinitionRevision int `json:"TaskDefinitionRevision,omitempty"` |
There was a problem hiding this comment.
I think this has to be *int. 0 is still a valid int and omitempty would not apply to it.
| // https://docs.aws.amazon.com/cli/latest/reference/ecs/describe-task-definition.html | ||
| // if anything bad happens, then 0 is returned, so you must omitempty to omit this from | ||
| // the metadata JSON output. | ||
| func parseTaskDefinitionRevision(task *api.Task, containerName string) int { |
There was a problem hiding this comment.
as per my comment in containermetadata/types.go, i think this has to return (*int, error). If there's an error, the pointer would not be set and omitempty will kick in.
| @@ -21,6 +21,7 @@ import ( | |||
|
|
|||
| go_dockerclient "github.com/fsouza/go-dockerclient" | |||
| gomock "github.com/golang/mock/gomock" | |||
| "github.com/aws/amazon-ecs-agent/agent/api" | |||
There was a problem hiding this comment.
I think the convention is to order import statements as: 1) native go packages, 2) packages from the same repo, 3) other packages, with an empty line between each group. So this new line should be in 21?
|
ping @jtoberon: what's going on with this PR? do we still need this merged? |
|
I'll probably pick this back up in 2 weeks or so. |
adnxn
changed the title
|
closing in favor of #1295 |
Summary
Expose two more metadata fields, task definition family and task definition
Implementation details
Retrieve the fields from the existing task struct, and copy them into the
metadata JSON.
Testing
make release)go build -out amazon-ecs-agent.exe ./agent)make test) passgo test -timeout=25s ./agent/...) passmake run-integ-tests) pass.\scripts\run-integ-tests.ps1) passmake run-functional-tests) pass.\scripts\run-functional-tests.ps1) passNew tests cover the changes: yes
Description for the changelog
Enhancement -Expose two more metadata fields, task definition family and task definition
Licensing
This contribution is under the terms of the Apache 2.0 License: yes