-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Short names for staged policies #10280
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
Short names for staged policies #10280
Conversation
| p.StorageType, | ||
| authorizer, | ||
| []string{}, | ||
| []string{"sgnp", "csgnp", "calicostagedglobalnetworkpolicies"}, |
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.
We should add calicostagedglobalnetworkpolicy.
|
I think we should add short names for |
8135f19 to
e79d543
Compare
matthewdupre
left a comment
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.
Some name tweaks
| p.StorageType, | ||
| authorizer, | ||
| []string{}, | ||
| []string{"csnp", "calicostagednetworkpolicy", "calicostagednetworkpolicies"}, |
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 think these names are a bit confusing: the resource kind is StagedNetworkPolicy, so these should allude to that. I think it's also preferable to have the s appear at the start. So here I think the good options would be "snp", "stagednetworkpolicy".
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.
And then maybe add "scnp", "stagedcaliconetworkpolicy"
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 think for this one we might be good with just the "snp", if we would want to add the calico differentiation we should follow the same order as for networkpolicy which has calico at front (caliconetworkpolicy)
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.
:( - so much inconsistency. Ah well. I think the names I'd like to see most are the ones that are just word->letter mappings from the kind in the same ordering.
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.
Yeah, agreed - the important one here is snp I think
| p.StorageType, | ||
| authorizer, | ||
| []string{}, | ||
| []string{"sgnp", "csgnp", "calicostagedglobalnetworkpolicies", "calicostagedglobalnetworkpolicy"}, |
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 don't think we should have the csgnp option (keep staged in front), and I don't think we need the Calico qualifier either since there is no k8s GlobalNetworkPolicy
(cherry picked from commit 7764bd5)
Description
Allow the use of short names for Staged Policies
StagedNetworkPolicy -> snp
StagedGlobalNetworkPolicy -> sgnp
StagedKubernetesNetworkPolicy -> sknp
Related issues/PRs
Todos
Release Note
Reminder for the reviewer
Make sure that this PR has the correct labels and milestone set.
Every PR needs one
docs-*label.docs-pr-required: This change requires a change to the documentation that has not been completed yet.docs-completed: This change has all necessary documentation completed.docs-not-required: This change has no user-facing impact and requires no docs.Every PR needs one
release-note-*label.release-note-required: This PR has user-facing changes. Most PRs should have this label.release-note-not-required: This PR has no user-facing changes.Other optional labels:
cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.