Skip to content

Bug 2082763: Drop the OperatorHub CR instance#245

Merged
openshift-merge-robot merged 1 commit into
openshift:masterfrom
njhale:fix/vendor-api
May 12, 2022
Merged

Bug 2082763: Drop the OperatorHub CR instance#245
openshift-merge-robot merged 1 commit into
openshift:masterfrom
njhale:fix/vendor-api

Conversation

@njhale
Copy link
Copy Markdown
Contributor

@njhale njhale commented Apr 13, 2022

  • Bump and vendor openshift/api to include a renaming of the OperatorHub
    CRD yaml. This will preclude the CRD from cluster-config-operator's
    CVO manifests and is part of an effort to make the marketplace-operator
    optional via OpenShift's capabilities feature.
    (openshift/api was bumped in another PR)
  • Drop the OperatorHub CR instance

@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 Apr 13, 2022
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Apr 13, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Comment thread empty-resources/0000_05_config-operator_02_operatorhub.cr.yaml Outdated
@openshift-ci openshift-ci Bot requested review from mfojtik and soltysh April 13, 2022 17:54
@njhale njhale marked this pull request as ready for review April 13, 2022 19:21
@openshift-ci openshift-ci Bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Apr 13, 2022
@njhale
Copy link
Copy Markdown
Contributor Author

njhale commented Apr 13, 2022

@openshift-ci openshift-ci Bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Apr 13, 2022
@openshift-ci openshift-ci Bot requested a review from tkashem April 13, 2022 19:23
@njhale
Copy link
Copy Markdown
Contributor Author

njhale commented Apr 19, 2022

/hold cancel

The PR this was blocked on has merged.

@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 19, 2022
@tylerslaton
Copy link
Copy Markdown

Approach looks good and the vendor changes came through well.

/lgtm

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

awgreene commented Apr 19, 2022

I support the changes here, but we will need to ask QE to update the OpenShift documentation which states that it ships with an operatorhub resource named cluster. Instead, we'll need this section to ask that the user manually create the resource.

I was a bit worried that Marketplace won't behave as expected if the default operatorhub resource is not present, but this code indicates that the default catalogSources would be created as expected.

I don't have approval permissions in this repo, but I'll give my support!
/approve

@njhale
Copy link
Copy Markdown
Contributor Author

njhale commented Apr 19, 2022

I support the changes here, but we will need to ask QE to update the OpenShift documentation which states that it ships with an operatorhub resource named cluster. Instead, we'll need this section to ask that the user manually create the resource.

I was a bit worried that Marketplace won't behave as expected if the default operatorhub resource is not present, but this code indicates that the default catalogSources would be created as expected.

I don't have approval permissions in this repo, but I'll give my support!
/approve

The operatorhub CRD is now included in marketplace as part of it's CVO manifests; i.e. it's all or nothing anyway.

@awgreene
Copy link
Copy Markdown
Contributor

I support the changes here, but we will need to ask QE to update the OpenShift documentation which states that it ships with an operatorhub resource named cluster. Instead, we'll need this section to ask that the user manually create the resource.
I was a bit worried that Marketplace won't behave as expected if the default operatorhub resource is not present, but this code indicates that the default catalogSources would be created as expected.
I don't have approval permissions in this repo, but I'll give my support!
/approve

The operatorhub CRD is now included in marketplace as part of it's CVO manifests; i.e. it's all or nothing anyway.

Ah I did not know that the cluster operatorhub CR was moved to the Marketplace repo, sgtm.

@njhale
Copy link
Copy Markdown
Contributor Author

njhale commented Apr 29, 2022

/retest-required

1 similar comment
@wking
Copy link
Copy Markdown
Member

wking commented Apr 30, 2022

/retest-required

@njhale
Copy link
Copy Markdown
Contributor Author

njhale commented May 2, 2022

/hold

I believe this is blocked on operator-framework/operator-marketplace#472

TL;DR, It seems like we're depending on an old version of openshift/api in marketplace, which means that the CRD manifest is old and is hitting some relatively new logic in CVO that prevents manifests w/o annotations from being merged. I suspect that bumping and revendoring openshift/api in marketplace will resolve this issue.

@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 May 2, 2022
@njhale
Copy link
Copy Markdown
Contributor Author

njhale commented May 3, 2022

/retest-required

@njhale
Copy link
Copy Markdown
Contributor Author

njhale commented May 3, 2022

/hold cancel

@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label May 4, 2022
@njhale njhale changed the title Bump(o/api): drop operatorhub crd yaml Drop the OperatorHub CR instance May 4, 2022
@exdx
Copy link
Copy Markdown

exdx commented May 4, 2022

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label May 4, 2022
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 4, 2022

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

@wking
Copy link
Copy Markdown
Member

wking commented May 8, 2022

/retitle Bug 2082763: Drop the OperatorHub CR instance

@openshift-ci openshift-ci Bot changed the title Drop the OperatorHub CR instance Bug 2082763: Drop the OperatorHub CR instance May 8, 2022
@openshift-ci openshift-ci Bot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels May 8, 2022
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 8, 2022

@njhale: This pull request references Bugzilla bug 2082763, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.11.0) matches configured target release for branch (4.11.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @jianzhangbjz

Details

In response to this:

Bug 2082763: Drop the OperatorHub CR instance

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 requested a review from jianzhangbjz May 8, 2022 05:36
@jianzhangbjz
Copy link
Copy Markdown

Hi, I couldn't test it before merging via the cluster-bot since it doesn't support setting the baselineCapabilitySet: None flag at the install time, please move it on, thanks!

@bparees
Copy link
Copy Markdown
Contributor

bparees commented May 10, 2022

Hi, I couldn't test it before merging via the cluster-bot since it doesn't support setting the baselineCapabilitySet: None flag at the install time, please move it on, thanks!

you can produce a payload from this PR using cluster-bot, and then do your own install w/ the None capset, right?

@perdasilva
Copy link
Copy Markdown

@kevinrizza Hey guys, what's missing for this to get merged? Manual testing by Jian? Could I ask that we please make this a priority. It would be great if we could get in asap, before Friday. Is there anything we can do to help expedite it?

@tkashem
Copy link
Copy Markdown
Contributor

tkashem commented May 11, 2022

/approve

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 11, 2022
@tkashem
Copy link
Copy Markdown
Contributor

tkashem commented May 11, 2022

holding it until qe finishes their testing, feel free to cancel the hold once qe is happy

/hold

@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 May 11, 2022
@bparees
Copy link
Copy Markdown
Contributor

bparees commented May 11, 2022

@jianzhangbjz are you able to test this by using cluster-bot to build a payload from this PR, and then install from that payload w/ the None capset?

@jianzhangbjz
Copy link
Copy Markdown

@bparees Hi, I'm PTO yesterday, will try it today, thanks!

@jianzhangbjz
Copy link
Copy Markdown

LGTM based on https://bugzilla.redhat.com/show_bug.cgi?id=2082763#c5
/lgtm

@jianzhangbjz
Copy link
Copy Markdown

/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 May 12, 2022
@jianzhangbjz
Copy link
Copy Markdown

@tkashem Could you help /unhold it? Thanks!

@openshift-merge-robot openshift-merge-robot merged commit 2faeea8 into openshift:master May 12, 2022
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 12, 2022

@njhale: All pull requests linked via external trackers have merged:

Bugzilla bug 2082763 has been moved to the MODIFIED state.

Details

In response to this:

Bug 2082763: Drop the OperatorHub CR instance

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

openshift-ci Bot commented May 12, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, bparees, exdx, jianzhangbjz, njhale, tkashem, tylerslaton

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

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. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants