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

[Multicast] Remove NetworkPolicyStats dependency of MulticastGroup API #5367

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

ceclinux
Copy link
Contributor

@ceclinux ceclinux commented Aug 8, 2023

Fixes #5329

@ceclinux ceclinux requested a review from antoninbas August 8, 2023 11:16
@ceclinux
Copy link
Contributor Author

ceclinux commented Aug 8, 2023

/test-multicast-e2e

@@ -91,20 +91,29 @@ func TestRESTList(t *testing.T) {
networkPolicyStatsEnabled bool
Copy link
Member

Choose a reason for hiding this comment

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

I think we should remove this factor from tests too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we remove this factor, we cannot test the result of MulticastGroup API is irrelevant to the NetworkPolicyStats feature because we have already asserted it. Not sure if this is good practice. Could you explain more?

Copy link
Member

Choose a reason for hiding this comment

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

Then the condition would be kept forever? Could it be confusing in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@ceclinux ceclinux force-pushed the mcast-no-depend-on-networkpolicystats branch from 56bbe4e to e926842 Compare August 10, 2023 09:44
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@antoninbas antoninbas added action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes. labels Aug 10, 2023
@antoninbas
Copy link
Contributor

@ceclinux could you backport this (ideally to 1.13, 1.12 and 1.11)?

@antoninbas
Copy link
Contributor

/test-all

@ceclinux
Copy link
Contributor Author

@ceclinux could you backport this (ideally to 1.13, 1.12 and 1.11)?

sure

@antoninbas
Copy link
Contributor

/test-all

@tnqn
Copy link
Member

tnqn commented Aug 15, 2023

/test-conformance

@tnqn
Copy link
Member

tnqn commented Aug 15, 2023

/test-networkpolicy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kubectl get multicastgroups requires NetworkPolicyStats feature to be enabled
3 participants