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

NPL code tidy-up for Antrea v2.0 #5943

Merged
merged 1 commit into from
Feb 28, 2024
Merged

Conversation

luolanzone
Copy link
Contributor

For #5070

@@ -137,6 +138,7 @@ annotation is deprecated. The array contains a single member, equal to the
`protocol` field.
The `protocols` field will be removed from Antrea for minor releases post March 2023,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @jianjuns @antoninbas @tnqn according to the doc, this field is deprecated almost a year, could you suggest if we should remove the field protocols in Antrea v2.0? Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I think we can remove it.

@luolanzone luolanzone changed the title [WIP]NPL code tidy-up for Antrea v2.0 NPL code tidy-up for Antrea v2.0 Feb 1, 2024
@XinShuYang
Copy link
Contributor

/test-windows-containerd-e2e

Comment on lines 139 to 136
The `protocols` field will be removed from Antrea for minor releases post March 2023,
as per our deprecation policy.
Starting from Antrea v2.0, the `protocols` field is removed.
Copy link
Member

Choose a reason for hiding this comment

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

I feel the whole content starting with "Starting from the Antrea v1.7 minor release, the protocols field ..." can be removed. There seems no much value to detail the history of a removed field when there is already a "Usage pre Antrea v1.7" section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

PodPort: npl.PodPort,
PodIP: pod.Status.PodIP,
Protocol: npl.Protocol,
Protocols: npl.Protocols,
Copy link
Member

Choose a reason for hiding this comment

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

We need to remove Protocols of rules.PodNodePort and stop using it in AddAllRules. Otherwise AddAllRules won't work, I'm curious why e2e tests didn't fail. #5917 can catch the issue but currently it has to expect wrong output.

Copy link
Contributor Author

@luolanzone luolanzone Feb 20, 2024

Choose a reason for hiding this comment

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

Removed.
The e2e codes are updated accordingly for the change in this PR and the PodNodePort is not used before in NPL e2e test cases.

@tnqn
Copy link
Member

tnqn commented Feb 20, 2024

#5917 has been merged, the PR needs to rebase on main and fix the new unit test.

Signed-off-by: Lan Luo <luola@vmware.com>
@luolanzone
Copy link
Contributor Author

@tnqn the unit test failure is fixed, could you take a look again? Thanks.

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

@tnqn
Copy link
Member

tnqn commented Feb 28, 2024

/test-all

@tnqn tnqn merged commit d3a0e72 into antrea-io:main Feb 28, 2024
50 of 54 checks passed
@tnqn tnqn mentioned this pull request Apr 17, 2024
@luolanzone luolanzone deleted the npl-cleanup branch April 22, 2024 02:46
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.

None yet

4 participants