-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
refactor netpol/policies.go #99696
refactor netpol/policies.go #99696
Conversation
/cc @jayunit100 @knabben |
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.
👍 👍 this looks awesome, I'm excited about it and totally onboard! Left a couple comments, up to you to decide whether to action them or not!
Will wait to approve until @jayunit100 @knabben and @abhiraut have had a look!!
🎉 🎉
/triage accepted |
d09cbea
to
565d74b
Compare
/retest |
565d74b
to
0e9d12d
Compare
/cc @knabben I remove all the Get function and use our new structured function to replace them. Most of Get funcs are call less 3 times even one time. New set func is flexible and enough to use. There is no neccessary for us to maintain these Get function, most of them are very Customized. And in genernal, using Set function, we can get a wanted network netpolicy less 4 COL. I dont think We need new function to gather them. |
0e9d12d
to
cd2af69
Compare
/retest |
@danwinship ^ are you ok with using ordering conventions to generate these policies? |
thanks Jorn , this does look cool... lets make sure other folks are ok w/ this refactor, the original KEP here has some history on this, we went back and forth on wether a DSL style approach would be valid but couldnt get consensus .... history here https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/1611-network-policy-validation |
/test pull-kubernetes-unit |
@jayunit100 do you mean |
waiting for @danwinship review on this one. /lgtm |
sorry, belatedly noticing this. I haven't paid any attention to what actually got done here so you probably don't want me offering opinions on the code at this point or I'll want you to rewrite the whole thing 🙂 |
so.. we had agreed this is a good refactor in the PR history (and the rewriting start :) /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JornShen, knabben The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
2 similar comments
/retest |
/retest |
/retest Review the full test history for this PR. Silence the bot with an |
/test pull-kubernetes-node-e2e |
/kind cleanup
cut down 400+ COL