-
Notifications
You must be signed in to change notification settings - Fork 19
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
Infer PolicyTypes if missing #50
Conversation
Pull Request Test Coverage Report for Build 5529027813
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost okey to me. Appreciated if you incorporate my comments. thanks.
pkg/server/server.go
Outdated
} | ||
} | ||
} | ||
var ingressEnable, egressEnable bool = getEnabledPolicyTypes(policy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about to change that? (it is just for shorten code with same meaning, so not critical)
ingressEnable, egressEnable := getEnabledPolicyTypes(policy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks!
|
@zeeke Appreciated your e2e test fix! Could you please rebase the PR to get the e2e fix? |
@zeeke I update the PR but e2e still failed. PTAL?
|
Sure, I'll have a look |
In cases where Spec.PolicyTypes is not specified, it should default to the existence of Ingress or Egress rules. Updating end2end tests to cover also this scenario. Signed-off-by: Andrea Panattoni <apanatto@redhat.com>
Signed-off-by: Andrea Panattoni <apanatto@redhat.com>
Some tests were missing a sleep after the test setup. I think some assertions were running before the server policy sync. |
@zeeke thank you for the quick fix! Let's merge that. |
In cases where Spec.PolicyTypes is not specified, it should default to the existence of Ingress or Egress rules.
Updating end2end tests to cover also this scenario.
Changes have been guided by the regular NetworkPolicy documentation: