Skip to content

Conversation

RishabhSaini
Copy link
Contributor

Update the hack/update-payload-crds.sh to generate the crds for PIS V1

Copy link
Contributor

openshift-ci bot commented Apr 1, 2025

Hello @RishabhSaini! 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/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 1, 2025
@djoshy
Copy link
Contributor

djoshy commented Apr 1, 2025

/lgtm

Looks good from MCO POV. As openshift/machine-config-operator#4934 has landed, the MCO now expects the v1 CRD to deployed instead of v1alpha1.

@openshift-ci openshift-ci bot requested review from deads2k and everettraven April 1, 2025 14:34
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 1, 2025
@RishabhSaini
Copy link
Contributor Author

/retest-required

1 similar comment
@RishabhSaini
Copy link
Contributor Author

/retest-required

@JoelSpeed
Copy link
Contributor

Since this adds the v1 types to the payload, does this then break the MCO code as it exists today? Is there a PR which we need to simulmerge that updates the MCO code to use the V1 API?

@RishabhSaini
Copy link
Contributor Author

Since this adds the v1 types to the payload, does this then break the MCO code as it exists today? Is there a PR which we need to simulmerge that updates the MCO code to use the V1 API?

Adapting MCO to use V1 has already merged: openshift/machine-config-operator#4934

@JoelSpeed
Copy link
Contributor

/lgtm

Given that it's already merged, I assume your tests are broken now (they should have blocked the merge no?)

This should get you back to working

@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
@djoshy
Copy link
Contributor

djoshy commented Apr 2, 2025

{ fail [github.com/openshift/origin/test/extended/operators/check_crd_status.go:190]: CRD pinnedimagesets.machineconfiguration.openshift.io has no 'status' element in its schema
Ginkgo exit error 1: exit with code 1}

The failures on the techpreview suites seem to be real 🤔

Given that it's already merged, I assume your tests are broken now (they should have blocked the merge no?)

I think PIS origin tests are still in the works(correct me if I'm wrong @RishabhSaini ), so it didn't block the merge unfortunately. We noticed these failures during manual tests, ideally we should've simulmerged with openshift/machine-config-operator#4934 😞

@JoelSpeed
Copy link
Contributor

/approve cancel

Do you have a status sub resource marked within the CRD?

@openshift-ci openshift-ci bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 2, 2025
@RishabhSaini
Copy link
Contributor Author

RishabhSaini commented Apr 2, 2025

{ fail [github.com/openshift/origin/test/extended/operators/check_crd_status.go:190]: CRD pinnedimagesets.machineconfiguration.openshift.io has no 'status' element in its schema
Ginkgo exit error 1: exit with code 1}

PIS.Status was removed as a part of this #2198

Do you have a status sub resource marked within the CRD?

No the only required field is Spec. The status subresource is removed in the CRDs in this PR

I will create another origin PR to add PIS to the exception list to the test here:
https://github.com/openshift/origin/blob/b1cde86316ef71820f6598e5a008d5c051d47e60/test/extended/operators/check_crd_status.go#L85C1-L106C1

@RishabhSaini
Copy link
Contributor Author

Here is the PR:
openshift/origin#29643

@RishabhSaini RishabhSaini changed the title Generate v1 crds for PIS MCO-1630: Generate v1 crds for PIS 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

openshift-ci-robot commented Apr 2, 2025

@RishabhSaini: This pull request references MCO-1630 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.

In response to this:

Update the hack/update-payload-crds.sh to generate the crds for PIS V1

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.

@RishabhSaini
Copy link
Contributor Author

/retest-required

@RishabhSaini
Copy link
Contributor Author

/retest-required

@RishabhSaini
Copy link
Contributor Author

RishabhSaini commented Apr 3, 2025

Since openshift/origin#29643 has merged and the tests are passing here. @JoelSpeed this is ready for review

Copy link
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

(still will need Joel's approval to merge)

@JoelSpeed
Copy link
Contributor

/approve

Copy link
Contributor

openshift-ci bot commented Apr 4, 2025

[APPROVALNOTIFIER] This PR is APPROVED

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

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 /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 4, 2025
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 84bcaa1 and 2 for PR HEAD 9e0a945 in total

1 similar comment
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 84bcaa1 and 2 for PR HEAD 9e0a945 in total

Copy link
Contributor

openshift-ci bot commented Apr 4, 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-azure 9e0a945 link false /test e2e-azure
ci/prow/okd-scos-e2e-aws-ovn 9e0a945 link false /test okd-scos-e2e-aws-ovn
ci/prow/e2e-gcp 9e0a945 link false /test e2e-gcp

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.

@hexfusion
Copy link
Contributor

/test e2e-aws-serial-techpreview

@openshift-merge-bot openshift-merge-bot bot merged commit aa88294 into openshift:master Apr 5, 2025
20 of 23 checks passed
@openshift-bot
Copy link

[ART PR BUILD NOTIFIER]

Distgit: ose-cluster-config-api
This PR has been included in build ose-cluster-config-api-container-v4.20.0-202504062308.p0.gaa88294.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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants