Skip to content
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

OCPVE-711: feat: add olm capability annotation #2105

Merged
merged 1 commit into from Oct 18, 2023

Conversation

eggfoobar
Copy link
Contributor

@eggfoobar eggfoobar commented Oct 5, 2023

OLM is an optional operator, we exclude OLM resources with this annotation if the monitoring operator ever becomes a capability, using a <monitoring_label>+OperatorLifecycleManager will work for both

Openshift has the capabilities feature documented in this enhancement, This allows us to annotate manifests for specific operators and make them optional during install. The reason that this PR exists is because the OperatorLifecycleManager is going to be optional in 4.15, the deployment of OperatorGroup fails during install, so we add this annotation and CVO will know how to handle it correctly.

  • I added CHANGELOG entry for this change.
  • No user facing changes, so no entry in CHANGELOG was needed.

/hold

Wait for CVO to be merged in first openshift/cluster-version-operator#971

OLM is an optional operator, we exclude OLM resources with this annotation
if the monitoring operator ever becomes a capability, using a <monitoring_label>+OperatorLifecycleManager will work for both

Signed-off-by: ehila <ehila@redhat.com>
@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 5, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Oct 5, 2023

@eggfoobar: This pull request references OCPVE-711 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 story to target the "4.15.0" version, but no target version was set.

In response to this:

OLM is an optional operator, we exclude OLM resources with this annotation if the monitoring operator ever becomes a capability, using a <monitoring_label>+OperatorLifecycleManager will work for both

  • I added CHANGELOG entry for this change.
  • No user facing changes, so no entry in CHANGELOG was needed.

/hold

Wait for CVO to be merged in first openshift/cluster-version-operator#971

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/test-infra repository.

@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 Oct 5, 2023
Copy link
Contributor

@danielmellado danielmellado left a comment

Choose a reason for hiding this comment

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

Hi, althought it seems pretty much straightforward... would you mind clarifying the need for this? I'd love to get the PR message expanded with some more context, thanks!

@eggfoobar
Copy link
Contributor Author

Hey @danielmellado, Sure thing! Openshift has the capabilities feature documented in this enhancement, This allows us to annotate manifests for specific operators and make them optional during install. The reason that this PR exists is because the OperatorLifecycleManager is going to be optional in 4.15, the deployment of OperatorGroup fails during install, so we add this annotation and CVO will know how to handle it correctly.

If all goods from your end, feel free to slap a lgtm/approve, we're getting final validation and I'll go ahead and unhold it once the change is in CVO.

@eggfoobar
Copy link
Contributor Author

/unhold

@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 Oct 17, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Oct 17, 2023

@eggfoobar: This pull request references OCPVE-711 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 story to target the "4.15.0" version, but no target version was set.

In response to this:

OLM is an optional operator, we exclude OLM resources with this annotation if the monitoring operator ever becomes a capability, using a <monitoring_label>+OperatorLifecycleManager will work for both

Openshift has the capabilities feature documented in this enhancement, This allows us to annotate manifests for specific operators and make them optional during install. The reason that this PR exists is because the OperatorLifecycleManager is going to be optional in 4.15, the deployment of OperatorGroup fails during install, so we add this annotation and CVO will know how to handle it correctly.

  • I added CHANGELOG entry for this change.
  • No user facing changes, so no entry in CHANGELOG was needed.

/hold

Wait for CVO to be merged in first openshift/cluster-version-operator#971

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/test-infra repository.

@danielmellado
Copy link
Contributor

@eggfoobar thanks for addressing my comment there and adding the clarification, it does lgtm but I'd like for @jan--f or @simonpasquier to take a look at this too, thanks!

@danielmellado
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 18, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielmellado, eggfoobar

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

openshift-ci bot commented Oct 18, 2023

@eggfoobar: all tests passed!

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/test-infra repository. I understand the commands that are listed here.

@openshift-ci openshift-ci bot merged commit 3cc8dd9 into openshift:master Oct 18, 2023
16 checks passed
@machine424
Copy link
Contributor

machine424 commented Oct 18, 2023

Hello @eggfoobar

Are you sure this is sufficient to avoid deploying that resource once a cluster disable OLM? I'm afraid this would implicitly re-enable OLM back:
cf https://issues.redhat.com/browse/OCPBUGS-18326?focusedId=23265807&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-23265807

I think this is documented in here https://github.com/openshift/enhancements/blob/cb81452fddf86c1099acd87610b88369cd6192db/enhancements/installer/component-selection.md#updates but maybe it's something worth mentioning in https://github.com/openshift/enhancements/tree/master/dev-guide/cluster-version-operator as well (I'm not sure, maybe I'm getting it wrong, but a more explicit doc will help in all cases)

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.

None yet

4 participants