Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

containermetadata: Metadata task definition fields #1295

Merged
merged 1 commit into from
Mar 22, 2018
Merged

containermetadata: Metadata task definition fields #1295

merged 1 commit into from
Mar 22, 2018

Conversation

haikuoliu
Copy link
Contributor

@haikuoliu haikuoliu commented Mar 14, 2018 •
edited
Loading

Summary

This is to pick up and finish pr #1122 to fix #1295.

Expose two more metadata fields, task definition family and task definition revision.

It also fixes some typos.

Implementation details

Retrieve the fields from the existing task struct, and copy them into the metadata JSON.

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: yes

Description for the changelog

Enhancement - Expose two more metadata fields, task definition family and task definition revision

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

aaithal
aaithal requested changes Mar 19, 2018
View reviewed changes
agent/containermetadata/parse_metadata.go Outdated
return Metadata{
cluster: manager.cluster,
taskMetadata: TaskMetadata{containerName: containerName, taskARN: taskARN},
taskMetadata: TaskMetadata{containerName: containerName,
taskARN: task.Arn, taskDefinitionFamily: task.Family,
Copy link
Contributor

Choose a reason for hiding this comment

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

please split this into multiple lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

agent/containermetadata/parse_metadata.go Outdated
return Metadata{
cluster: manager.cluster,
taskMetadata: TaskMetadata{containerName: containerName, taskARN: taskARN},
taskMetadata: TaskMetadata{containerName: containerName,
taskARN: task.Arn, taskDefinitionFamily: task.Family,
Copy link
Contributor

Choose a reason for hiding this comment

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

please split this into multiple lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

agent/containermetadata/types.go Outdated
ContainerInstanceARN string `json:"ContainerInstanceARN,omitempty"`
TaskARN string `json:"TaskARN,omitempty"`
TaskDefinitionFamily string `json:"TaskDefinitionFamily,omitempty"`
TaskDefinitionRevision int `json:"TaskDefinitionRevision,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

0 is not the same as the field not being set. You'd need to make this *int for "omitempty" to be effective.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I was thinking if the task.Version is empty or it's not a number, strconv.Atoi will return a 0, note that the task version starts at 1, so 0 is not a valid task version. And according to https://golang.org/pkg/encoding/json/, "omitempty" will omit the fields with 0, nil, etc, so in this way int will work.
I'll make it *int for better readability.

agent/containermetadata/parse_metadata.go Outdated
// https://docs.aws.amazon.com/cli/latest/reference/ecs/describe-task-definition.html
// if anything bad happens, then 0 is returned (0 is not a valid task definition revision number),
// so you must omitempty to omit this from the metadata JSON output.
func parseTaskDefinitionRevision(task *api.Task, containerName string) int {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need this method? Why not just use

amazon-ecs-agent/agent/api/task.go

Line 76 in edc3e26

Version string
?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the type of the task.Version 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).
So to be consistent with the CLI, I make it a number here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will make taskDefinitionRevision a string after further discussion.

@adnxn adnxn mentioned this pull request Mar 20, 2018
Closed
8 tasks
sharanyad
sharanyad approved these changes Mar 20, 2018
View reviewed changes
Copy link
Contributor

@sharanyad sharanyad left a comment

Choose a reason for hiding this comment

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

code lgtm. If you could update the copyright year in the files' header, that would be great.

agent/containermetadata/parse_metadata_test.go
metadata = newManager.parseMetadata(nil, mockTask, mockContainerName)
assert.Equal(t, metadata.taskMetadata.taskDefinitionFamily, mockTaskDefinitionFamily, "Expected task definition family "+mockTaskDefinitionFamily)
assert.Equal(t, metadata.taskMetadata.taskDefinitionRevision, mockTaskDefinitionRevision, "Expected task definition revision "+mockTaskDefinitionRevision)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: empty line at the eof

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@sharanyad sharanyad added this to the 1.18.0 milestone Mar 21, 2018
@haikuoliu haikuoliu modified the milestones: 1.18.0, 1.17.3 Mar 21, 2018
@aaithal
Copy link
Contributor

aaithal commented Mar 21, 2018

can you please make sure modify the CHANGELOG.md file as well?

aaithal
aaithal reviewed Mar 21, 2018
View reviewed changes
CHANGELOG.md Outdated
@@ -1,6 +1,7 @@
# Changelog

## 1.17.2
* Enhancement - Expose two more metadata fields, task definition family and task definition revision [#1295](https://github.com/aws/amazon-ecs-agent/pull/1295)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be under 1.17.3-dev. You'll see it if you rebase it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, thanks for pointing out, I just rebased and updated the change log again

aaithal
aaithal approved these changes Mar 22, 2018
View reviewed changes
Copy link
Contributor

@aaithal aaithal left a comment

Choose a reason for hiding this comment

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

please make sure that you update the changelog entry as per comments. lgtm otherwise!

CHANGELOG.md Outdated
@@ -1,6 +1,7 @@
# Changelog

## 1.17.3-dev
* Enhancement - Expose two more metadata fields, task definition family and task definition revision [#1295](https://github.com/aws/amazon-ecs-agent/pull/1295)
Copy link
Contributor

Choose a reason for hiding this comment

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

Because we have multiple ways to get metadata, can you also specify that this is for 'container metadata file'? Example:Enhancement - Expose task definition family and task definition revision in container metadata file...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will change it accordingly.

@haikuoliu
containermetadata: expose task definition family and revision as meta…
c600d69 
…data fields

The task struct already has this information, so it's just a matter
of plumbing it through.

This commit also fixes some typos.
@haikuoliu haikuoliu merged commit 6f16e3c into aws:dev Mar 22, 2018
@haikuoliu haikuoliu deleted the branch_metadata branch April 12, 2018 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sharanyad sharanyad sharanyad approved these changes

@aaithal aaithal aaithal approved these changes

@adnxn adnxn Awaiting requested review from adnxn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.17.3
Development

Successfully merging this pull request may close these issues.
3 participants
@haikuoliu @aaithal @sharanyad