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.

Sign up for GitHub

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

Jump to bottom

Added the new rook bundle and removed rook csv from ocs-operator #2492

Merged
merged 3 commits into from
Mar 20, 2024
Merged

Added the new rook bundle and removed rook csv from ocs-operator #2492

merged 3 commits into from
Mar 20, 2024

Conversation

Nikhil-Ladha
Copy link
Member

  • Added the rook bundle and updated the ocs-operator catalog to inlcude the rook bundle
  • Removed the rook csv, crds from ocs-operator.

@Nikhil-Ladha Nikhil-Ladha force-pushed the remove-rook-csv branch 3 times, most recently from 089a5c7 to 8e701d7 Compare March 5, 2024 10:58
@Nikhil-Ladha Nikhil-Ladha mentioned this pull request Mar 6, 2024
Open
malayparida2000
malayparida2000 reviewed Mar 6, 2024
View reviewed changes
hack/source-manifests.sh Outdated Show resolved Hide resolved
tools/csv-merger/csv-merger.go Show resolved Hide resolved
tools/csv-merger/csv-merger.go Show resolved Hide resolved
hack/common.sh Outdated Show resolved Hide resolved
@Nikhil-Ladha
Copy link
Member Author

Depends on: red-hat-storage/rook#584, red-hat-storage/rook#581

@Nikhil-Ladha
Copy link
Member Author

Tested out cluster deployment and everything is working fine with the new rook-ceph-operator-bundle deployment.

@Nikhil-Ladha Nikhil-Ladha force-pushed the remove-rook-csv branch 2 times, most recently from 8f206df to 4a27dac Compare March 14, 2024 09:12
@petr-balogh petr-balogh mentioned this pull request Mar 14, 2024
Closed
@malayparida2000 malayparida2000 mentioned this pull request Mar 15, 2024
Closed
iamniting
iamniting reviewed Mar 15, 2024
View reviewed changes
tools/csv-merger/csv-merger.go
@@ -600,8 +335,6 @@ func generateUnifiedCSV() *csvv1.ClusterServiceVersion {
// Used by UI to validate user uploaded metadata
// Metadata is used to connect to an external cluster
ocsCSV.Annotations["external.features.ocs.openshift.io/validation"] = `{"secrets":["rook-ceph-operator-creds", "rook-csi-rbd-node", "rook-csi-rbd-provisioner"], "configMaps": ["rook-ceph-mon-endpoints", "rook-ceph-mon"], "storageClasses": ["ceph-rbd"], "cephClusters": ["monitoring-endpoint"]}`
// Injecting the RHCS exporter script present in Rook CSV
ocsCSV.Annotations["external.features.ocs.openshift.io/export-script"] = rookCSV.GetAnnotations()["externalClusterScript"]
Copy link
Member

Choose a reason for hiding this comment

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

@bipuladh @SanjalKatiyar FYI, because we read this script in the UI. We will have to change the source of the script in the UI.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks !
can you please create corresponding console BZ with the details (assuming PR will be merged in 4.16) ?? cc @Nikhil-Ladha @iamniting

Copy link
Member Author

Choose a reason for hiding this comment

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

Since, this is for 4.16 I think having a JIRA story should be enough to take this in rather than creating a separate bz for it. Does that work for you @SanjalKatiyar ?

Copy link
Contributor

@SanjalKatiyar SanjalKatiyar Mar 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

it is a small change from UI, so we could have taken it up after feature freeze as well (hence the BZ) + this way, QE can test this flow once from the UI too !
or is this change already part of some 4.16 epic (in which case it will make sense to create a task/story under it) ??

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's a part of 4.16 epic that's the reason I suggested to have a JIRA story instead of a bz.
Ref: https://issues.redhat.com/browse/RHSTOR-5267

@Nikhil-Ladha Nikhil-Ladha force-pushed the remove-rook-csv branch 2 times, most recently from 12920ef to 3b4885a Compare March 18, 2024 08:54
@malayparida2000 malayparida2000 mentioned this pull request Mar 19, 2024
Merged
@Nikhil-Ladha
Copy link
Member Author

/retest

1 similar comment
@Nikhil-Ladha
Copy link
Member Author

/retest

@iamniting
Copy link
Member

Can you add catalog/rook-ceph-bundle.yaml in the gitignore

@Nikhil-Ladha
Copy link
Member Author

Can you add catalog/rook-ceph-bundle.yaml in the gitignore

It's already added

@Nikhil-Ladha Nikhil-Ladha force-pushed the remove-rook-csv branch 2 times, most recently from b57c83b to 0bf997c Compare March 19, 2024 10:40
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 19, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 19, 2024
iamniting
iamniting requested changes Mar 19, 2024
View reviewed changes
Copy link
Member

@iamniting iamniting left a comment

Choose a reason for hiding this comment

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

Can we separate generated and non-generated changes from the second commit.

@Nikhil-Ladha
Copy link
Member Author

Can we separate generated and non-generated changes from the second commit.

Done

@Nikhil-Ladha Nikhil-Ladha force-pushed the remove-rook-csv branch 4 times, most recently from 117a789 to f3e4b5b Compare March 19, 2024 12:34
@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 19, 2024
@iamniting
Copy link
Member

/test ocs-operator-bundle-e2e-aws

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 19, 2024
@iamniting
Copy link
Member

/test ocs-operator-bundle-e2e-aws

@subhamkrai
Copy link
Contributor

@Nikhil-Ladha @iamniting any idea how we can get the CI green and merge the PR sooner?

@iamniting
Copy link
Member

@Nikhil-Ladha @iamniting any idea how we can get the CI green and merge the PR sooner?

We are working on it. We will try to get it in ASAP.

@Nikhil-Ladha
Copy link
Member Author

/retest

@Nikhil-Ladha
Add rook bundle to ocs-operator catalog
d6b50f2 
Add rook bundle to ocs-operator-catalog and update
the relevant script files to inlcude the rook bundle image name
and add rook bundle deployment script

Signed-off-by: Nikhil-Ladha <[email protected]>
@Nikhil-Ladha
Remove rook csv,crds generation from ocs-operator
207d768 
Remove rook csv,crds generation from ocs-operator
and update the csv-merger tool

Signed-off-by: Nikhil-Ladha <[email protected]>
@Nikhil-Ladha
Add generated changes for rook csv removal
41bc21e 
Add generated changes for rook csv removal from ocs-operator

Signed-off-by: Nikhil-Ladha <[email protected]>
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 20, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 20, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamniting, Nikhil-Ladha

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-merge-bot openshift-merge-bot bot merged commit d8c4789 into red-hat-storage:main Mar 20, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@malayparida2000 malayparida2000 malayparida2000 left review comments

@SanjalKatiyar SanjalKatiyar SanjalKatiyar left review comments

@subhamkrai subhamkrai subhamkrai left review comments

@iamniting iamniting iamniting approved these changes

@travisn travisn Awaiting requested review from travisn

@umangachapagain umangachapagain Awaiting requested review from umangachapagain

Assignees

@iamniting iamniting

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Nikhil-Ladha @iamniting @subhamkrai @travisn @SanjalKatiyar @malayparida2000 @openshift-merge-robot