Skip to content

Conversation

jackyalbo
Copy link
Contributor

@jackyalbo jackyalbo commented Feb 24, 2020

As part of not being dependent on community operators in downstream
fix to BZ1798571
upstream NooBaa will still require lib_bucket_provisionser as a dependency

@openshift-ci-robot
Copy link

Hi @jackyalbo. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-robot openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 24, 2020
@umangachapagain
Copy link
Contributor

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 25, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 25, 2020
Copy link
Contributor

@umangachapagain umangachapagain left a comment

Choose a reason for hiding this comment

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

Is this fix only for ocs-operator case?
This change should ideally come from NooBaa operator itself. We should not be overriding it here.

@guymguym
Copy link
Contributor

@umangachapagain That can't come from NooBaa itself.
Upstream noobaa will still require those CRDs and get them through OLM dependency on the lib-bucket-provisioner operator (CRD only operator).
However downstream we don't want OCS to depend on a community operator and therefore we decide to own those CRDs by OCS.

@jackyalbo
Copy link
Contributor Author

/retest

Copy link
Contributor

@umangachapagain umangachapagain left a comment

Choose a reason for hiding this comment

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

Will the new CRDs be persisted across make gen-latest-csv ?
I have a feeling it will not be included during new bundle creation as we fetch our CRDs during build from NooBaa and Rook. We might need to add these CRDs to /deploy/crds/ directory.

@guymguym
Copy link
Contributor

Yes it does. We still get it the same way, just now we patch it from required to owned.

@guymguym
Copy link
Contributor

But @umangachapagain you’re right that @jackyalbo should make sure that those crd files themselves are really updated from noobaa operator during CSV merge, and not statically stored in OCS git.

@jackyalbo
Copy link
Contributor Author

they are updated by the operator - was added by running "make gen-latest-csv"

Copy link
Contributor

@umangachapagain umangachapagain left a comment

Choose a reason for hiding this comment

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

/lgtm

Just would be neat to have "go.mod / go.sum" changes in a separate commit.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 26, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 26, 2020
@jackyalbo
Copy link
Contributor Author

fixed to 2 separate commits

ocsCSV.Spec.CustomResourceDefinitions.Owned = append(ocsCSV.Spec.CustomResourceDefinitions.Owned, csvStruct.Spec.CustomResourceDefinitions.Owned...)
ocsCSV.Spec.CustomResourceDefinitions.Required = append(ocsCSV.Spec.CustomResourceDefinitions.Required, csvStruct.Spec.CustomResourceDefinitions.Required...)

for _, definition := range csvStruct.Spec.CustomResourceDefinitions.Required {
Copy link
Member

Choose a reason for hiding this comment

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

Is this in place because the NooBaa CSV we're importing isn't updated to reflect this change? If so, shouldn't the change be in NooBaa?

@umangachapagain
Copy link
Contributor

umangachapagain commented Feb 27, 2020

@umangachapagain That can't come from NooBaa itself.
Upstream noobaa will still require those CRDs and get them through OLM dependency on the lib-bucket-provisioner operator (CRD only operator).

If lib-bucket-provisioner operator is gone, how will NooBaa operator function upstream?

@guymguym
Copy link
Contributor

@umangachapagain lib-bucket-provisioner is only removed from cumminity-operators for Openshift, not from upstream-community-operators for Kubernetes.

As part of not being dependent on community operators BZ1798571

Signed-off-by: jackyalbo <jalbo@redhat.com>
Signed-off-by: jackyalbo <jalbo@redhat.com>
@openshift-ci-robot
Copy link

@jackyalbo: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/ocs-operator-e2e-aws fd99074 link /test ocs-operator-e2e-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@jarrpa
Copy link
Member

jarrpa commented Mar 5, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackyalbo, jarrpa, umangachapagain

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 5, 2020
@openshift-merge-robot openshift-merge-robot merged commit e021528 into red-hat-storage:master Mar 5, 2020
@obnoxxx
Copy link
Contributor

obnoxxx commented Mar 25, 2020

/cherry-pick release-4.4

@openshift-cherrypick-robot

@obnoxxx: new pull request created: #458

In response to this:

/cherry-pick release-4.4

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.

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. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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.

8 participants