Skip to content

WIP: [CCXDEV-15259]: promote InsightConfigAPI and OnDemandDataGather to v1 #2448

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

WIP: [CCXDEV-15259]: promote InsightConfigAPI and OnDemandDataGather to v1 #2448

wants to merge 5 commits into from

Conversation

opokornyy
Copy link
Contributor

No description provided.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 12, 2025
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Aug 12, 2025

Hello @opokornyy! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

@openshift-ci openshift-ci bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 12, 2025
@openshift-ci openshift-ci bot requested review from deads2k and everettraven August 12, 2025 14:24
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Aug 12, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: opokornyy
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@opokornyy
Copy link
Contributor Author

/retest

@opokornyy
Copy link
Contributor Author

/test verify-feature-promotion

@opokornyy opokornyy force-pushed the CCXDEV-15259-io-v1 branch 2 times, most recently from d7b46b0 to dce787e Compare August 15, 2025 09:48
@opokornyy opokornyy changed the title WIP: promote InsightConfigAPI and OnDemandDataGather to v1 WIP: [CCXDEV-15259]: promote InsightConfigAPI and OnDemandDataGather to v1 Aug 15, 2025
JoelSpeed
JoelSpeed reviewed Aug 18, 2025
View reviewed changes
config/v1/types_insights.go Outdated
Spec InsightsDataGatherSpec `json:"spec,omitempty,omitzero"`
// status holds observed values from the cluster. They may not be overridden.
// +optional
Status *InsightsDataGatherStatus `json:"status,omitempty,omitzero"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is status: {} valid? Usually it isn't something we would expect, so this shouldn't need to be a pointer

@opokornyy opokornyy force-pushed the CCXDEV-15259-io-v1 branch 2 times, most recently from 382e32d to c47f3e9 Compare August 19, 2025 08:08
@opokornyy
feat(insights): add v1 version
c01c100 
Signed-off-by: Ondrej Pokorny <[email protected]>
@opokornyy
feat(config): add v1 version for insights config
3b94abc 
Signed-off-by: Ondrej Pokorny <[email protected]>
@opokornyy
fix: linter errors
abc890b 
Signed-off-by: Ondrej Pokorny <[email protected]>
@opokornyy
fix: integration tests
502bf0a 
Signed-off-by: Ondrej Pokorny <[email protected]>
everettraven
everettraven reviewed Aug 19, 2025
View reviewed changes
config/v1/types_insights.go
// +kubebuilder:validation:XValidation:rule="self.all(x, self.exists_one(y, x == y))",message="dataPolicy items must be unique"
// +listType=atomic
// +optional
DataPolicy []DataPolicyOption `json:"dataPolicy,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it mean for this to be dataPolicy: [] vs just omitting the dataPolicy field? We generally prefer to avoid having more than one way to "say" the same thing.

IIUC, the current validation of allowing a length of 0 for this means that unstructured clients can set dataPolicy: [] but is not something that structured clients would ever be able to set because of the omitempty.

We should make sure that unstructured and structured clients behave the same here.

IIRC, that means:

  • IF dataPolicy: [] is semantically different than omission of the field, swap the omitempty to omitzero (allows marshalling of the [] but not null values)
  • ELSE make MinItems validation to be 1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double checked with @JoelSpeed and am making a correction to:

IF dataPolicy: [] is semantically different than omission of the field, swap the omitempty to omitzero (allows marshalling of the [] but not null values)

This should actually be making the type *[]DataPolicyOption and keeping the omitempty because that sets us up for more idiomatic Go nil checking behavior where this field is actually referenced.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With that I am running into the linting error:

optionalfields: field DataPolicy does not allow the zero value. The field does not need to be a pointer. (kubeapilinter)
	DataPolicy *[]DataPolicyOption `json:"dataPolicy,omitempty"`

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you also change the MinItems validation? I'd expect a change to only one or the other.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it mean for this to be dataPolicy: [] vs just omitting the dataPolicy field? We generally prefer to avoid having more than one way to "say" the same thing.

This is the important part of the conversation to answer first, and it drives the other decisions.

I believe the answer is that dataPolicy: [] is semantically equivalent to being omitted.

In which case, we would say that we don't want to allow dataPolicy: [] to be persisted as a valid value.

In which case, it should have a non-zero +kubebuilder:validation:MinProperties, which will then force the list to be []T not *[]T.

The ordering of answering the questions influences the correct end result.

@opokornyy are you in agreement that the following is true?

`dataPolicy: []` is semantically equivalent to being omitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, I have set the // +kubebuilder:validation:MinItems=1 for dataPolicy and now it does not require to use a pointer.

@opokornyy
fix: dataPolicy linter error
d29ee57 
Signed-off-by: Ondrej Pokorny <[email protected]>
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Aug 29, 2025

@opokornyy: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-ovn-techpreview d29ee57 link true /test e2e-aws-ovn-techpreview
ci/prow/e2e-aws-serial-techpreview-2of2 d29ee57 link true /test e2e-aws-serial-techpreview-2of2
ci/prow/e2e-aws-serial-techpreview-1of2 d29ee57 link true /test e2e-aws-serial-techpreview-1of2
ci/prow/e2e-aws-ovn-hypershift d29ee57 link true /test e2e-aws-ovn-hypershift
ci/prow/e2e-aws-ovn d29ee57 link true /test e2e-aws-ovn

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k Awaiting requested review from deads2k

2 more reviewers

@JoelSpeed JoelSpeed JoelSpeed left review comments

@everettraven everettraven everettraven left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@opokornyy @JoelSpeed @everettraven