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

Refactor: Encapsulate net opts loading in internalLabels #3793

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

motinsa
Copy link
Contributor

@motinsa motinsa commented Dec 29, 2024

Context

The create.go file contained a TODO to formalize the way network
options are loaded into internalLabels. The previous implementation
manually assigned each field, which made it harder to maintain and extend.

Solution

  • Introduced a new method LoadNetOpts in the internalLabels struct.
  • Encapsulated the logic for loading network options, improving
    maintainability and extensibility.

Impact

This change is backward-compatible and does not affect existing functionality.

Signed-off-by: Pedro Aibar [email protected]

@@ -732,6 +726,16 @@ func withInternalLabels(internalLabels internalLabels) (containerd.NewContainerO
return containerd.WithAdditionalContainerLabels(m), nil
}

// LoadNetOpts loads network options into InternalLabels.
func (il *internalLabels) LoadNetOpts(opts types.NetworkOptions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

LoadNetOpts is a public function. Could you please clarify why it is necessary for this function to be public rather than private?"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making LoadNetOpts public was unintentional. I've already updated the function to be private to align with its intended use case.

Copy link
Member

@fahedouch fahedouch left a comment

Choose a reason for hiding this comment

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

Thanks

@fahedouch fahedouch added this to the v2.0.3 milestone Jan 10, 2025
@fahedouch fahedouch merged commit 751b33b into containerd:main Jan 10, 2025
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants