-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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.
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
e2e: unmarshal json into container summaries #9547
Conversation
Signed-off-by: Nick Sieger <[email protected]>
publishers := service["Publishers"].([]interface{}) | ||
if service["Name"] == "e2e-ps-busybox-1" { | ||
publishers := service.Publishers | ||
if service.Name == "e2e-ps-busybox-1" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the services expected to be returned in a consistent order? If so, this loop may not be needed, and possibly it could be a single compare between an expected slice (in the right order)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I guess there may be other fields that need to be ignored in the comparing in that case 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the order may be consistent, I'm not familiar enough with the way the summaries are generated to know for sure. I was a little surprised that they were reported in alpha order instead of declaration order in the compose file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are any documented assertions about order then I'll update this to match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was mostly a "drive-by" comment from my side; perhaps others more familiar with the codebase could provide more details 😅 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :) much more readable
What I did
Oops! This didn't quite make it into #9545 before it was merged. I like how this reads even though it puts a dependency on api (or maybe that's not desired?).
Related issue
(not mandatory) A picture of a cute animal, if possible in relation with what you did