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

Parse defaultTransferNetwork annotation with namespace #948

Merged

Conversation

yaacov
Copy link
Member

@yaacov yaacov commented Jul 7, 2024

Description
The pull request introduces a conditional check to determine the format of the network key. The key can be in two formats:

  • <namespace>/<name>
  • <name>

Explanation
The updated code checks if the network key contains a /.

  • If it does, it splits the network key into namespace and name using strings.SplitN(network, "/", 2).
  • If it doesn't, it assumes the key is just the name and assigns the Namespace using mutator.plan.Spec.TargetNamespace.

Summary

  • Before: The code always used mutator.plan.Spec.TargetNamespace for the Namespace.
  • After: The code checks the format of the network key. If it includes a namespace (<namespace>/<name>), it splits and uses the specified namespace. Otherwise, it defaults to using mutator.plan.Spec.TargetNamespace.

This enhancement ensures that the correct namespace is used based on the format of the network key, providing greater flexibility and correctness in handling network annotations.

@yaacov yaacov added the bug Something isn't working label Jul 7, 2024
@yaacov
Copy link
Member Author

yaacov commented Jul 7, 2024

@ahadas @bennyz please review

Copy link

codecov bot commented Jul 7, 2024

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.

Project coverage is 16.30%. Comparing base (a8aeb93) to head (707b8bb).

Files Patch % Lines
...webhooks/mutating-webhook/mutators/plan-mutator.go 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #948      +/-   ##
==========================================
+ Coverage   15.83%   16.30%   +0.47%     
==========================================
  Files         108      103       -5     
  Lines       19973    19332     -641     
==========================================
- Hits         3162     3153       -9     
+ Misses      16530    15900     -630     
+ Partials      281      279       -2     
Flag Coverage Δ
unittests 16.30% <0.00%> (+0.47%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yaacov yaacov force-pushed the parse-default-transfer-netwrork-with-namepsace branch 2 times, most recently from 4a2f843 to 44d776f Compare July 7, 2024 07:45
Copy link
Member

@ahadas ahadas left a comment

Choose a reason for hiding this comment

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

the logic that was introduced in #608 seems to be duplicated here, it would be better to reduce the code duplication, but the change itself looks good

Name: network,
var key client.ObjectKey

// Netwoek key format <namespace>/<name> or <name>, in case the network key does not
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Netwoek key format <namespace>/<name> or <name>, in case the network key does not
// Network key format <namespace>/<name> or <name>, in case the network key does not

@yaacov yaacov force-pushed the parse-default-transfer-netwrork-with-namepsace branch 2 times, most recently from 414350f to d74883b Compare July 7, 2024 08:53
tests/suit/forklift_test.go Outdated Show resolved Hide resolved
@yaacov yaacov force-pushed the parse-default-transfer-netwrork-with-namepsace branch 5 times, most recently from 039c762 to 195391f Compare July 7, 2024 11:51
@yaacov yaacov force-pushed the parse-default-transfer-netwrork-with-namepsace branch from 195391f to 707b8bb Compare July 7, 2024 12:08
@ahadas ahadas changed the title Parse defaultTransferNetwork anotation whith namespace Parse defaultTransferNetwork annotation whith namespace Jul 7, 2024
@ahadas ahadas merged commit e0e7020 into kubev2v:main Jul 7, 2024
11 of 12 checks passed
@ahadas ahadas added this to the 2.6.3 milestone Jul 7, 2024
@ahadas ahadas removed this from the 2.6.3 milestone Jul 7, 2024
@yaacov yaacov changed the title Parse defaultTransferNetwork annotation whith namespace Parse defaultTransferNetwork annotation with namespace Jul 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants