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

build: export ROOK_IMAGE env variable #598

Merged
merged 2 commits into from
Mar 22, 2024
Merged

Conversation

Nikhil-Ladha
Copy link
Member

Update script to export ROOK_IMAGE env variable so that it can be updated with custom values during the build

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

@Nikhil-Ladha Nikhil-Ladha requested a review from subhamkrai March 22, 2024 03:51
@Nikhil-Ladha Nikhil-Ladha force-pushed the add-rook-image-env-variable branch 2 times, most recently from a9f64e2 to 39e7562 Compare March 22, 2024 04:06
Update script to export ROOK_IMAGE env variable so that
it can be updated with custom values during the build

Signed-off-by: Nikhil-Ladha <[email protected]>
add generated changes for csv

Signed-off-by: Nikhil-Ladha <[email protected]>
@Nikhil-Ladha Nikhil-Ladha force-pushed the add-rook-image-env-variable branch from 39e7562 to 1398c5a Compare March 22, 2024 04:20
@Nikhil-Ladha
Copy link
Member Author

/retest

@@ -429,7 +429,7 @@ metadata:
}
]
relatedImages:
- image: docker.io/rook/ceph:master
- image: rook/ceph:master

Choose a reason for hiding this comment

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

Suggested change
- image: rook/ceph:master
- image: rook/ceph:master

it was copied from what ocs-operator had any issue with previously or it is just an improvement

Copy link
Member Author

Choose a reason for hiding this comment

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

Just an improvement, technically all these updates were handled by the csv-merger tool in ocs-operator. But, here we are updating via sed command so made it similar to the other image field used in deployement spec such that both the fields are updated during the csv-gen.

@@ -1863,7 +1863,7 @@ metadata:
name: rook-ceph-operator.v4.15.0
namespace: placeholder
relatedImages:
- image: docker.io/rook/ceph:master
- image: docker.io/rook/ceph:v1.13.0.399.g9c0d795e2

Choose a reason for hiding this comment

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

Suggested change
- image: docker.io/rook/ceph:v1.13.0.399.g9c0d795e2
- image: rook/ceph:v1.13.0.399.g9c0d795e2

seems like this also needs to be updated?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the result of above update. This keeps the container image and relatedImages field same in the csv

Choose a reason for hiding this comment

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

@Nikhil-Ladha I see this in related images image: rook/ceph:master which is not same as above.

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, that's the placeholder.
The actual value is updated via the gen-csv file. over here: https://github.com/red-hat-storage/rook/blob/master/build/csv/csv-gen.sh#L53

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 22, 2024
@subhamkrai subhamkrai requested a review from sp98 March 22, 2024 07:11
@Nikhil-Ladha
Copy link
Member Author

/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 Mar 22, 2024
@Nikhil-Ladha
Copy link
Member 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 Mar 22, 2024
@subhamkrai
Copy link

Based on this comment #599 (comment) seems like we are good with the PR

@subhamkrai subhamkrai mentioned this pull request Mar 22, 2024
6 tasks
@Nikhil-Ladha
Copy link
Member Author

@sp98 can you please review this PR?
TIA :)

Copy link

openshift-ci bot commented Mar 22, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

The full list of commands accepted by this bot can be found 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

@subhamkrai subhamkrai merged commit 0395b58 into master Mar 22, 2024
48 of 49 checks passed
@Nikhil-Ladha Nikhil-Ladha deleted the add-rook-image-env-variable branch March 28, 2024 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants