-
Notifications
You must be signed in to change notification settings - Fork 542
add .status.relatedObjects to clusterversion to get CVO references #2339
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
base: master
Are you sure you want to change the base?
Conversation
Hello @deads2k! Some important instructions when contributing to openshift/api: |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k 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 |
// 2. operand namespaces | ||
// +optional | ||
// +openshift:enable:FeatureGate=CVORelatedObjects | ||
RelatedObjects []ObjectReference `json:"relatedObjects,omitempty"` |
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.
+listType=map
and appropriate listMapKey
missing
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.
+listType=map and appropriate listMapKey missing
Will do. Consideration for our future, will we fix the existing usages too?
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.
If they're in status, I would expect we are already doing the right thing and so should be fine to fix it
I also assume these would ratchet 🤔
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.
Oh very neat. I think you can only make map keys on required fields and not all the fields are logically required. I might go with atomic (single writer of this thing anyway)
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 you can only make map keys on required fields and not all the fields are logically required
Yes I agree, the required ones are group
, name
and resource
, so potential overlap on namespace
?
If you don't have the listType enforcing the uniqueness, how about CEL?
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.
For reference, the incantation is
+kubebuilder:validation:XValiadtion:rule="self.all(x, self.exists_one(y, x == y))",message="items in relatedObjects should be unique"
// 2. operand namespaces | ||
// +optional | ||
// +openshift:enable:FeatureGate=CVORelatedObjects | ||
RelatedObjects []ObjectReference `json:"relatedObjects,omitempty"` |
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.
List should set a sensible maximum length, given intended use case
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.
List should set a sensible maximum length, given intended use case
I'm so glad I insisted on these linters :)
/hold I want to see this work all the way through the client before committing. I seem to recall that map keys are required to be required. |
// `oc adm inspect` honors this field in any type to navigate and collect related data. | ||
// Common uses are: | ||
// 1. operator namespaces | ||
// 2. operand namespaces |
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 obviously want to include the openshift-cluster-version
namespace for our operator, but "operand" gets a bit sticky with the CVO. Do we do just our own namespace on the thin end, and build out CVO logging as needed to help debug things where we don't capture enough to debug a resource we're failing to reconcile? Or all ~1k resources release-manifests are asking us to manage on the thick end? Or pick some middle ground like "asking for all ClusterOperators is easy, and likely covers reporting on most of the resources we care about"? Or...?
c65bf41
to
8807b6d
Compare
API LGTM if we are happy with the 100 resource limit |
741cb7c
to
b85e830
Compare
b85e830
to
8b7ce44
Compare
@deads2k: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This will allow
oc adm inspect
to gather pertinent data. The CVO will need to grow a control loop that reconciles this new field. My preference will be server-side-apply, perhaps even the super naive "apply this manifest" approach./assign @wking