Skip to content

Conversation

@perdasilva
Copy link

@perdasilva perdasilva commented Oct 15, 2024

  • Promotes operator/v1alpha1 OLM to operator/v1 OLM

Next PR: #2066

@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 Oct 15, 2024
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 15, 2024

Hello @perdasilva! 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 Oct 15, 2024
@openshift-ci openshift-ci bot requested review from bparees and knobunc October 15, 2024 10:47
@perdasilva perdasilva force-pushed the perdasilva/bump/operators-api-olm-v1 branch 2 times, most recently from 22a471d to c535238 Compare October 15, 2024 13:25
@openshift-ci openshift-ci bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 15, 2024
@perdasilva
Copy link
Author

verify-crd-schema is failing because observedGeneration is not in OperatorCondition. We've reached out on the #forum-api-review to see if this is something we should override. slack thread

@perdasilva perdasilva force-pushed the perdasilva/bump/operators-api-olm-v1 branch from c535238 to c69832c Compare October 16, 2024 08:32
@JoelSpeed
Copy link
Contributor

@perdasilva You need to ensure your project is checked out under an appropriate $GOPATH or, not run the protobuf generator

Looking at the PR, I think protobuf generation is not required, so perhaps revert all the proto related changes and run the updates with PROTO_OPTIONAL=1

@perdasilva perdasilva force-pushed the perdasilva/bump/operators-api-olm-v1 branch from c69832c to cdeb00e Compare October 16, 2024 09:46
@perdasilva
Copy link
Author

perdasilva commented Oct 16, 2024

Thanks @JoelSpeed ^^ I just saw the message on the api forum XD - should be fixed up now (I hope)

@perdasilva perdasilva force-pushed the perdasilva/bump/operators-api-olm-v1 branch from cdeb00e to 14fe41e Compare October 16, 2024 09:59
@perdasilva
Copy link
Author

/retest

2 similar comments
@perdasilva
Copy link
Author

/retest

@perdasilva
Copy link
Author

/retest

@perdasilva perdasilva changed the title [WIP] ⚠️ Bump OLM API to v1 [HOLD] OPRUN-3402: ⚠️ Bump OLM API to v1 Oct 18, 2024
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Oct 18, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Oct 18, 2024

@perdasilva: This pull request references OPRUN-3402 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 spike to target the "4.18.0" version, but no target version was set.

Details

In response to this:

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 openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 18, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Oct 18, 2024

@perdasilva: This pull request references OPRUN-3402 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 spike to target the "4.18.0" version, but no target version was set.

Details

In response to this:

  • Promotes operator/v1alpha1 OLM to operator/v1 OLM
  • Removes operator/v1alpha1 OLM

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

openshift-ci-robot commented Oct 18, 2024

@perdasilva: This pull request references OPRUN-3402 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 spike to target the "4.18.0" version, but no target version was set.

Details

In response to this:

  • Promotes operator/v1alpha1 OLM to operator/v1 OLM
  • Removes operator/v1alpha1 OLM

Next PR: #2066

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.

@perdasilva perdasilva changed the title [HOLD] OPRUN-3402: ⚠️ Bump OLM API to v1 OPRUN-3402: ⚠️ Bump OLM API to v1 Oct 21, 2024
@perdasilva
Copy link
Author

/jira refresh

@openshift-ci-robot
Copy link

openshift-ci-robot commented Oct 21, 2024

@perdasilva: This pull request references OPRUN-3402 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 task to target the "4.18.0" version, but no target version was set.

Details

In response to this:

/jira refresh

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.

@perdasilva
Copy link
Author

@JoelSpeed This should be ready for review ^^

The failure in verify-crd-schema is:

 	could not run schemacheck generator for group/version operator.openshift.io/v1: 
		error in operator/v1/zz_generated.crd-manifests/0000_10_operator-lifecycle-manager_01_olms.crd.yaml: ConditionsMustHaveProperSSATags: crd/olms.operator.openshift.io version/v1 field/^.status.condition must define valid condition properties: observedGeneration attribute is missing
		error in operator/v1/zz_generated.featuregated-crd-manifests/olms.operator.openshift.io/AAA_ungated.yaml: ConditionsMustHaveProperSSATags: crd/olms.operator.openshift.io version/v1 field/^.status.condition must define valid condition properties: observedGeneration attribute is missing
+ echo This verifier checks all files that have changed. In some cases you may have changed or renamed a file that already contained api violations, but you are not introducing a new violation. In such cases it is appropriate to /override the failing CI job.
This verifier checks all files that have changed. In some cases you may have changed or renamed a file that already contained api violations, but you are not introducing a new violation. In such cases it is appropriate to /override the failing CI job. 

as discussed here, it should be ok to override.

(cc @joelanford)

@perdasilva perdasilva force-pushed the perdasilva/bump/operators-api-olm-v1 branch from 1c6a903 to 4248dd0 Compare October 28, 2024 07:55
@JoelSpeed
Copy link
Contributor

When bumping an API from v1alpha1 to v1, we must keep the old types around for some time. We need to be able to generate clients for both v1alpha1 and v1 simultaneously to allow users to build tooling versions that might span both GA and pre-GA

Please restore the v1alpha1 versions, and then I'll review the v1 API again to see if there's anything we might need to update prior to GA

@perdasilva perdasilva force-pushed the perdasilva/bump/operators-api-olm-v1 branch 2 times, most recently from 14fe41e to 820ae85 Compare October 28, 2024 15:35
@openshift-ci-robot
Copy link

openshift-ci-robot commented Oct 28, 2024

@perdasilva: This pull request references OPRUN-3402 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 task to target the "4.18.0" version, but no target version was set.

Details

In response to this:

  • Promotes operator/v1alpha1 OLM to operator/v1 OLM

Next PR: #2066

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.

@perdasilva
Copy link
Author

When bumping an API from v1alpha1 to v1, we must keep the old types around for some time. We need to be able to generate clients for both v1alpha1 and v1 simultaneously to allow users to build tooling versions that might span both GA and pre-GA

Please restore the v1alpha1 versions, and then I'll review the v1 API again to see if there's anything we might need to update prior to GA

@JoelSpeed Thanks for clarifying that. We really weren't sure if it would be ok given the support requirements, but didn't factor in other technical considerations. I've gone ahead and restored v1alpha1.

@perdasilva
Copy link
Author

/jira refresh

@openshift-ci-robot
Copy link

openshift-ci-robot commented Oct 30, 2024

@perdasilva: This pull request references OPRUN-3402 which is a valid jira issue.

Details

In response to this:

/jira refresh

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.

// +kubebuilder:subresource:status
// +openshift:api-approved.openshift.io=https://github.com/openshift/api/pull/1504
// +openshift:file-pattern=cvoRunLevel=0000_10,operatorName=operator-lifecycle-manager,operatorOrdering=01
// +kubebuilder:validation:XValidation:rule="self.metadata.name == 'cluster'",message="olm is a singleton, .metadata.name must be 'cluster'"
Copy link
Contributor

Choose a reason for hiding this comment

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

Has this been tested and we are happy that it works?

Copy link
Author

Choose a reason for hiding this comment

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

yup yup - we have a test for it https://github.com/openshift/api/pull/2061/files#diff-34775f0b3bbcc2a64f685c6fc398e13b3fae20851398b2b5a455726630673a01R21

I've also gone ahead and manually tested on a clusterbot. Looks good ^^

@JoelSpeed
Copy link
Contributor

Is there a related feature gate that also needs to be promoted?

What is the path to GA for this feature?

Do we have enough time to complete the path to GA prior to branch cutting?

@perdasilva perdasilva force-pushed the perdasilva/bump/operators-api-olm-v1 branch from 820ae85 to 3f3cf66 Compare November 6, 2024 18:17
@openshift-ci openshift-ci bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 6, 2024
Signed-off-by: Per Goncalves da Silva <[email protected]>
@perdasilva perdasilva force-pushed the perdasilva/bump/operators-api-olm-v1 branch from 3f3cf66 to 565c25d Compare November 7, 2024 14:57
@JoelSpeed
Copy link
Contributor

/lgtm

/override ci/prow/verify-crd-schema

Existing failures about observed generation cannot be resolved now

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2024
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed, perdasilva

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 Nov 7, 2024
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 7, 2024

@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-crd-schema

Details

In response to this:

/lgtm

/override ci/prow/verify-crd-schema

Existing failures about observed generation cannot be resolved now

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.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 7, 2024

@perdasilva: all tests passed!

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-merge-bot openshift-merge-bot bot merged commit d37bb9f into openshift:master Nov 7, 2024
@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.18.0-202411071810.p0.gd37bb9f.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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants