-
Notifications
You must be signed in to change notification settings - Fork 89
Add v1beta2 changes to IBMPowerVSImage #2412
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: arshadd-b 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 |
Hi @arshadd-b. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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-sigs/prow repository. |
9632fae
to
a3756f6
Compare
5d46687
to
eaf2b42
Compare
eaf2b42
to
1be332d
Compare
0956e60
to
322a3dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, I will take a another look once you addressed the comments.
Overall lets revisit the log statement and make them better.
/ok-to-test |
75af036
to
7d51cd4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM! minor comments added
infrav1.IBMPowerVSImageReadyCondition, | ||
infrav1.IBMPowerVSImageReadyV1Beta2Condition, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you share why we need both of these conditions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In conditions in v1beta2 we have set two conditions one is ready and one is imageReady
v1beta2:
Conditions:
Last Transition Time: 2025-07-16T13:45:25Z
Message: * ImageReady: ImageQueued
Observed Generation: 1
Reason: NotReady
Status: False
Type: Ready
Last Transition Time: 2025-07-16T13:45:25Z
Message:
Observed Generation: 1
Reason: ImageQueued
Status: False
Type: ImageReady
Last Transition Time: 2025-07-16T13:45:27Z
Message:
Observed Generation: 1
Reason: Ready
Status: True
Type: WorkspaceReady
infrav1.IBMPowerVSImageReadyCondition here is Ready
infrav1.IBMPowerVSImageReadyV1Beta2Condition here is ImageReady condition
// Before computing ready condition, make sure that ImageReady is always set. | ||
// NOTE: This is required because v1beta2 conditions comply to guideline requiring conditions to be set at the | ||
// first reconcile. | ||
if c := v1beta2conditions.Get(ibmPowerVSImage, infrav1.IBMPowerVSImageReadyV1Beta2Condition); c == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this check needed? is it necessary to set the check for the condition on every reconcile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will check this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Amulyam24 we don't need it in this case.
666478f
to
7399a47
Compare
7399a47
to
7ce6672
Compare
What this PR does / why we need it:
This PR consists of following changes
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #2389
Special notes for your reviewer:
/area provider/ibmcloud
Release note: