Skip to content

Refactor: Encapsulate net opts loading in internalLabels - #3793

Merged
fahedouch merged 2 commits into
containerd:mainfrom
motinsa:main
Jan 10, 2025
Merged

fahedouch merged 2 commits into
containerd:mainfrom
motinsa:main

Conversation

@motinsa

@motinsa motinsa commented Dec 29, 2024

Copy link
Copy Markdown
Contributor

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 pedroaibarmarco@gmail.com

Signed-off-by: Pedro Aibar <75799784+motinsa@users.noreply.github.com>
Comment thread pkg/cmd/container/create.go Outdated
}

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

Copy link
Copy Markdown
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
Copy Markdown
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.

Signed-off-by: Pedro Aibar <pedroaibarmarco@gmail.com>

@fahedouch fahedouch left a comment

Copy link
Copy Markdown
Member

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
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