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.

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

setPlatformOptions: Change signature #1771

Merged
merged 1 commit into from
Dec 27, 2022

Conversation

dcantah
Copy link
Member

@dcantah dcantah commented Dec 27, 2022

This changes setPlatformOptions' signature to avoid passing back a fresh copy of an internalLabels structure when we could just pass in a pointer and have the function set whats needed. This additionally removes the opts parameter as the naming is a bit odd for this. To me, reading the function name it'd seem as if we're going to make use of the opts in the body (e.g. actually apply/set them) but we just append to the slice internally. I've changed to returning a new slice that makes it more obvious that all we're doing internally is tallying up each platforms specific options and returning them.

This changes `setPlatformOptions`' signature to avoid passing back a fresh
copy of an internalLabels structure when we could just pass in a pointer
and have the function set whats needed. This additionally removes the
opts parameter as the naming is a bit odd for this. To me, reading the
function name it'd seem as if we're going to make use of the opts in the
body (e.g. actually apply/set them) but we just append to the slice
internally. I've changed to returning a new slice that makes it more obvious
that all we're doing internally is tallying up each platforms specific
options and returning them.

Signed-off-by: Danny Canter <danny@dcantah.dev>
@dcantah dcantah marked this pull request as ready for review December 27, 2022 04:50
@dcantah dcantah mentioned this pull request Dec 27, 2022
@AkihiroSuda AkihiroSuda merged commit b876456 into containerd:main Dec 27, 2022
@AkihiroSuda AkihiroSuda added this to the v1.2.0 milestone Dec 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants