-
Notifications
You must be signed in to change notification settings - Fork 260
feat: [NPM] num ACL rules for v2 & update existing metrics #1223
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
Conversation
| return "nil NPMNetworkPolicy" | ||
| } | ||
| itemStrings := make([]string, 0, len(netPol.ACLs)) | ||
| for _, item := range netPol.ACLs { |
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.
I get that we already had this but not sure how I feel about using string, overriding the String() method can lead to lost or misinterpreted information since it's rarely pointed to during an investigation
for example if we add a new property to NPMNetwork and try to print write to stdout, the dev would need to make sure to update this method to write the new property, string() can be a lot of maintenance
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.
agree, implementing String() does feel a little Java to me 🙂
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.
Discussed this internally and team decided to change this func to PrettyPrint() and will be tackled in a separate PR
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.
resolved in #1235
The following updates with UTs: