-
Notifications
You must be signed in to change notification settings - Fork 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
Port and AllocatedPortMapping msgpack omitempty #23980
Conversation
"boom": &Port{ | ||
Label: "boom_port", | ||
}, | ||
"boom": []string{"boom_port"}, |
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.
these diff tests are for task.config
, which doesn't actually know what a Port
is. for the docker driver, for example, ports
is a list of strings (which arguably is already tested with bam
above here), or a MapStrInt
port_map
(baz
).
maybe Port
here made sense at some point in the past, but doesn't seem right to me now.
to optimize log entries with empty fields, which TestPlanNormalize checks
d97e612
to
005c67b
Compare
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!
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
to optimize log entries with empty fields, which TestPlanNormalize checks.
split out from #23956 so this can be backported.