-
Notifications
You must be signed in to change notification settings - Fork 398
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
🌱 pkg/../types_apiexport: add name, namespace #2222
Conversation
05ed6af
to
5c70973
Compare
b85e80b
to
296e804
Compare
I cannot reproduce this locally 🤔 |
Does it fail 100% in CI? |
@stevekuznetsov so far twice but only on github 🤔 A local |
296e804
to
2473654
Compare
CI is unhappy, #2278 , below
|
2473654
to
54149c6
Compare
Flake on #2281 |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ncdc, stevekuznetsov 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 |
// This is mutually exclusive with resourceSelector. | ||
// +required | ||
// +kubebuilder:default=false | ||
All bool `json:"all"` |
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.
why not omitempty? IMO having "all" all the time is ugly.
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.
fixed, set +optional
and omitemtpy
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.
note: omitempty
implies +optional
. I debugged half of a day in controller-tools to find out.
// If "name" is unset, all objects from the namespace are being claimed. | ||
// | ||
// +required | ||
Namespace string `json:"namespace"` |
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.
why not omit empty?
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 why not optional?
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.
being required does not extend to more selector types.
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.
fixed, however how do we want to validate this, using CEL?
Summary
This adds name,namespace fields to permission claims so we can work independently on reverse permission claim authz and narrowly scoped permissionclaims by @nrb.
Related issue(s)
Relates to #2089
Relates to #1937