chore(VpcPeering): remove duplicated code that load local and remote KCP Network #1478
Conversation
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
tmilos77
requested changes
Oct 13, 2025
vandjelk
changed the title
tmilos77
requested changes
Oct 14, 2025
There was a problem hiding this comment.
The PR seems as incomplete with flaws in the logic, would say a makeover if required
- errors other then NotFound are not handled
- NotFound error should be communicated in the status
- nil errors logged in case of deletion
- whole peering flow interrupted with forget when peering deleted
vandjelk
force-pushed
the
peering/load-kcp-networks
branch
from
c639255 to
6f57c72
Compare
tmilos77
requested changes
Oct 22, 2025
Co-authored-by: Milos Tomic <[email protected]>
Co-authored-by: Milos Tomic <[email protected]>
Co-authored-by: Milos Tomic <[email protected]>
Co-authored-by: Milos Tomic <[email protected]>
tmilos77
requested changes
Oct 22, 2025
There was a problem hiding this comment.
cloud provider specific part can not ignore and continue if local/remote network are not set, either
- optimistically assume they are set since common part called the provider specific part
- defensively assert they are not nil and log huge common.ErrLogical if so and return immediately
| // remote client can't be created if remote network is not found | ||
| if state.remoteNetwork == nil { | ||
| return nil, nil | ||
| if state.RemoteNetwork() == nil { |
There was a problem hiding this comment.
The whole point of the common load local/remote network refactored out to the common side is that it ensures those objects. If it's a guarantee then it shouldn't be checked, and even less ignored if none is set. If the common part is ensuring local/remote network as they are required then this can not ignore if they are not set.
Either
- ensure that common part will not call the provider part and local/remote network will always be loaded and optimistically just assume they are set
- defensively log huge common.ErrLogical if any of them is nil and STOP FURTHER PROCESSING since preconditions are not met
There was a problem hiding this comment.
Unnecessary nil checks removed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Description
Changes proposed in this pull request:
Related issue(s)