daemon/cluster/executor: simplify handling of Network Attachments#49343
Merged
akerouanton merged 4 commits intomoby:masterfrom Jan 27, 2025
Merged
daemon/cluster/executor: simplify handling of Network Attachments#49343akerouanton merged 4 commits intomoby:masterfrom
akerouanton merged 4 commits intomoby:masterfrom
Conversation
c is used as name for the containerConfig receiver; remove the intermediate variable so that we don't shadow it. There's no bug here, because a new variable is created; just to prevent confusion and to make linters happier. Signed-off-by: Sebastiaan van Stijn <[email protected]>
All of this function only referenced the Network field in the attachment; use an intermediate variable to make the code less repetitive. Signed-off-by: Sebastiaan van Stijn <[email protected]>
This method was called in a loop, iterating over the container config's network-attachments. It was defined as a method, but only to lookup the same attachment we just iterated over existed, and to obtain a copy. As there were no goroutines involved, dereferencing should not be an issue and with Go 1.22+, dereferencing loop vars is no longer needed at all, so we can change this method to a regular constructor; also removing the redundant error-return in the process. Signed-off-by: Sebastiaan van Stijn <[email protected]>
…velope The Network field is the only field used from the NetworkAttachment within this code. Remove the NetworkAttachment envelope, and store the Network field directly instead. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
added
status/2-code-review
area/networking
robmry
approved these changes
Jan 27, 2025
akerouanton
approved these changes
Jan 27, 2025
thaJeztah
commented
Jan 27, 2025
Comment on lines
+69
to
+76
| // It looks like using a map is only for convenience, but not used | ||
| // for validation, nor for looking up the network by name. The name | ||
| // is part of the Network's properties (Network.Spec.Annotations.Name), | ||
| // and effectively only used for debugging; we should consider to | ||
| // change it to a slice. | ||
| // | ||
| // TODO(thaJeztah): should this check for empty and duplicate names? | ||
| c.networks[attachment.Network.Spec.Annotations.Name] = attachment.Network |
Member
Author
There was a problem hiding this comment.
If one of you has thoughts in this, let me know as well; I left it as a TODO here, as it was mostly something I was wondering if it would (should've been) checked on here, but I left it at that (for now)
Contributor
There was a problem hiding this comment.
My thought was just that it looks like the map isn't needed and a check would be right. But, changing it seems a bit risky without knowing this code well (which I don't) ... because maybe something's working by luck or judgement with a duplicate or empty name, in a case that's not expected or not tested.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
daemon/cluster/executor: networkCreateRequest don't shadow config
c is used as name for the containerConfig receiver; remove the intermediate
variable so that we don't shadow it. There's no bug here, because a new
variable is created; just to prevent confusion and to make linters happier.
daemon/cluster/executor: networkCreateRequest: slight DRY cleanup
All of this function only referenced the Network field in the attachment;
use an intermediate variable to make the code less repetitive.
daemon/cluster/executor: networkCreateRequest: not a method
This method was called in a loop, iterating over the container config's
network-attachments. It was defined as a method, but only to lookup
the same attachment we just iterated over existed, and to obtain a copy.
As there were no goroutines involved, dereferencing should not be an issue
and with Go 1.22+, dereferencing loop vars is no longer needed at all,
so we can change this method to a regular constructor; also removing the
redundant error-return in the process.
daemon/cluster/executor: containerConfig: store Network instead of envelope
The Network field is the only field used from the NetworkAttachment within
this code. Remove the NetworkAttachment envelope, and store the Network
field directly instead.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)