handlers/v1: add json tags to response structs #1473
Conversation
agent/handlers/v1/response.go
Outdated
| @@ -46,7 +46,7 @@ type TasksResponse struct { | |||
|
|
|||
| // ContainerResponse is the schema for the container response JSON object | |||
| type ContainerResponse struct { | |||
| DockerID string | |||
| DockerID string `json:"DockerId"` | |||
There was a problem hiding this comment.
same comment as #1471 (comment):
I'd also suggest adding unit tests that unmarshal raw strings and validate each of the fields in the unmarshalled object with the expected one
There was a problem hiding this comment.
Sure, there is a TODO in the summary that indicates I will add tests for it.
Will add it soon.
There was a problem hiding this comment.
@aaithal I would rather use a functional test that consumes the endpoint metadata for this instead of a unit test on the unmarshaller. I think a functional test will raise more warning flags about things like this breaking in the future.
There was a problem hiding this comment.
I'd vote for both instead of one or the other. You'd know immediately (during development) if you're breaking contracts if you add unit tests.
There was a problem hiding this comment.
i think in this case neither option would have helped because the tests were also changed with the field renames in the original PR. i cant think of any way to really help prevent this other than having more eyes reviewing. lmk if you have any other ideas though. 😕
i do agree we should add both unit and functional tests though for this change.
|
@haikuoliu: please add json tags for the other fields as well. |
agent/handlers/v1/response_test.go
Outdated
| ) | ||
|
|
||
| func TestTaskResponse(t *testing.T) { | ||
| expectedTaskResponseJSONString := |
There was a problem hiding this comment.
Instead of testing for string exactness, this should preferably test for the shape of the data: the expected keys and their values (so as to not test serialization/deserialization string format). This certainly handles the case at hand, but isn't very maintainable.
There was a problem hiding this comment.
It's worth noting that adding fields or reordering (through any cause) will make these fields unaligned and test fail.
|
@aaithal @adnxn @petderek Instead of unmarshalling raw string, I choose to marshal the struct and then compare it with the raw string, because I found that when I unmarshalled the string, it'll be marshalled successfully when either json tag matches or struct attribute matches, and it's case insensitive, so it cannot prevent the I will add functional test in a separate PR, so please review, thanks! |
|
@jahkeup I have addressed your comments and added the changelog. |
haikuoliu
force-pushed
the
branch_fix_handler
branch
from
fbb640a to
6a5ae42
Compare
haikuoliu
force-pushed
the
branch_fix_handler
branch
from
6a5ae42 to
98d92d8
Compare
Summary
This PR adds a json tag to
DockerIDin v1 handler response, so that when it's marshalled the json text of thisDockerIDattribute will beDockerId, this is to fix a bug (introduced in this PR) where we provide a different container response for v1 handler in Agent 1.19.0, and it breaks the customers who are using v1 handler.Will add functional tests in a separate PR.
Implementation details
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
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.