Skip to content

MCO-1631: Remove "[sig-arch][Early] CRDs for openshift.io should have a status in the CRD schema"#29643

Merged
openshift-merge-bot[bot] merged 2 commits intoopenshift:mainfrom
RishabhSaini:exceptionPIS
Apr 2, 2025
Merged

MCO-1631: Remove "[sig-arch][Early] CRDs for openshift.io should have a status in the CRD schema"#29643
openshift-merge-bot[bot] merged 2 commits intoopenshift:mainfrom
RishabhSaini:exceptionPIS

Conversation

@RishabhSaini
Copy link
Copy Markdown
Contributor

@RishabhSaini RishabhSaini commented Apr 2, 2025

"[sig-arch][Early] CRDs for openshift.io should have a status in the CRD schema" is no longer relevant.

We only want to be checking for:
If a CRD has a "status" field in the schema but no subresource.status defined. (which causes several problems). Its covered by an existing test "[sig-arch][Early] CRDs for openshift.io should have subresource.status". Hence leaving no need for the prior test to exist.

This work blocks openshift/api#2257, which is necessary to ship PIS as a feature for 4.19

@RishabhSaini
Copy link
Copy Markdown
Contributor Author

/retest

"imagecontentpolicies.config.openshift.io",
"imagecontentsourcepolicies.operator.openshift.io",
"machineconfigs.machineconfiguration.openshift.io",
"pinnedimagesets.machineconfiguration.openshift.io",
Copy link
Copy Markdown
Contributor

@deads2k deads2k Apr 2, 2025

Choose a reason for hiding this comment

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

/hold

don't do this. You could add checking to ensure that if no status subresource exists it's only ok if no status exists, but this opens up to severe errors around permissioning and semantics we cannot fix.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the original goal of the testing was

they aren't required to have status. Some objects don't. But if they have status in the schema, they must have a status subresource

#26954 (comment)

if the implementation got confused, that can be resolved. But if a status subelement exists, it must have a subresource for it.

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 2, 2025
@deads2k
Copy link
Copy Markdown
Contributor

deads2k commented Apr 2, 2025

Discussed in slack and going into past history found that this test was an unneeded side quest: #26954 (comment) . Rather than exception, remove the entire test requiring a status stanza in the schema.

We must keep the test that ensures every CRD with a status stanza also has a status subresource and no new exceptions should appear there.

/hold cancel

@openshift-ci openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 2, 2025
@RishabhSaini RishabhSaini changed the title Add PIS CR to the Exception list for Status Test Remove "[sig-arch][Early] CRDs for openshift.io should have a status in the CRD schema" Apr 2, 2025
Copy link
Copy Markdown
Contributor

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

LGTM based on the discussion with @deads2k in slack: https://redhat-internal.slack.com/archives/CE4L0F143/p1743606075623449

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 2, 2025
@deads2k
Copy link
Copy Markdown
Contributor

deads2k commented Apr 2, 2025

/approve

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Apr 2, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, everettraven, RishabhSaini

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

The pull request process is described here

Details 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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 2, 2025
@RishabhSaini RishabhSaini changed the title Remove "[sig-arch][Early] CRDs for openshift.io should have a status in the CRD schema" MCO-1631: Remove "[sig-arch][Early] CRDs for openshift.io should have a status in the CRD schema" Apr 2, 2025
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 2, 2025
@openshift-ci-robot
Copy link
Copy Markdown

openshift-ci-robot commented Apr 2, 2025

@RishabhSaini: This pull request references MCO-1631 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.19.0" version, but no target version was set.

Details

In response to this:

"[sig-arch][Early] CRDs for openshift.io should have a status in the CRD schema" is no longer relevant.

We only want to be checking for:
If a CRD has a "status" field in the schema but no subresource.status defined. (which causes several problems). Its covered by an existing test "[sig-arch][Early] CRDs for openshift.io should have subresource.status". Hence leaving no need for the prior test to exist.

This work blocks openshift/api#2257, which is necessary to ship PIS as a feature for 4.19

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 openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot
Copy link
Copy Markdown

/retest-required

Remaining retests: 0 against base HEAD b1cde86 and 2 for PR HEAD f4a2ff8 in total

@openshift-merge-bot openshift-merge-bot Bot merged commit f8b7258 into openshift:main Apr 2, 2025
33 of 54 checks passed
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Apr 2, 2025

@RishabhSaini: 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-vsphere-ovn-etcd-scaling f4a2ff8 link false /test e2e-vsphere-ovn-etcd-scaling
ci/prow/e2e-aws-ovn-upgrade f4a2ff8 link false /test e2e-aws-ovn-upgrade
ci/prow/e2e-azure-ovn-etcd-scaling f4a2ff8 link false /test e2e-azure-ovn-etcd-scaling
ci/prow/e2e-aws-ovn-etcd-scaling f4a2ff8 link false /test e2e-aws-ovn-etcd-scaling
ci/prow/e2e-metal-ipi-ovn f4a2ff8 link false /test e2e-metal-ipi-ovn
ci/prow/e2e-agnostic-ovn-cmd f4a2ff8 link false /test e2e-agnostic-ovn-cmd
ci/prow/e2e-aws-disruptive f4a2ff8 link false /test e2e-aws-disruptive
ci/prow/e2e-gcp-fips-serial f4a2ff8 link false /test e2e-gcp-fips-serial
ci/prow/e2e-aws-ovn-kube-apiserver-rollout f4a2ff8 link false /test e2e-aws-ovn-kube-apiserver-rollout
ci/prow/e2e-gcp-ovn-etcd-scaling f4a2ff8 link false /test e2e-gcp-ovn-etcd-scaling
ci/prow/e2e-openstack-ovn f4a2ff8 link false /test e2e-openstack-ovn
ci/prow/okd-e2e-gcp f4a2ff8 link false /test okd-e2e-gcp
ci/prow/e2e-aws-ovn-single-node-upgrade f4a2ff8 link false /test e2e-aws-ovn-single-node-upgrade
ci/prow/e2e-aws-csi f4a2ff8 link false /test e2e-aws-csi
ci/prow/e2e-openstack-serial f4a2ff8 link false /test e2e-openstack-serial
ci/prow/e2e-gcp-disruptive f4a2ff8 link false /test e2e-gcp-disruptive
ci/prow/e2e-vsphere-ovn-dualstack-primaryv6 f4a2ff8 link false /test e2e-vsphere-ovn-dualstack-primaryv6

Full PR test history. Your PR dashboard.

Details

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.

@openshift-bot
Copy link
Copy Markdown
Contributor

[ART PR BUILD NOTIFIER]

Distgit: openshift-enterprise-tests
This PR has been included in build openshift-enterprise-tests-container-v4.19.0-202504030332.p0.gf8b7258.assembly.stream.el9.
All builds following this will include this PR.

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants