Skip to content

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

arshadd-b
Copy link
Contributor

What this PR does / why we need it:

This PR consists of following changes

  • Adopt CAPI v1beta2 status changes
  • Standardize the controller flow by utilising CAPI provided helper functions
  • Removed usage of deprecated github.com/pkg/errors package.

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

  1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Add v1beta2 changes to IBMPowerVSImage

@k8s-ci-robot k8s-ci-robot added area/provider/ibmcloud Issues or PRs related to ibmcloud provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 1, 2025
Copy link

netlify bot commented Jul 1, 2025

Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

Name Link
🔨 Latest commit 7ce6672
🔍 Latest deploy log https://app.netlify.com/projects/kubernetes-sigs-cluster-api-ibmcloud/deploys/6881a4d707983c0008679085
😎 Deploy Preview https://deploy-preview-2412.cluster-api-ibmcloud.sigs.k8s.io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: arshadd-b
Once this PR has been reviewed and has the lgtm label, please assign mkumatag for approval. For more information see the Code Review Process.

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

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 1, 2025
@k8s-ci-robot
Copy link
Contributor

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 /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-sigs/prow repository.

@arshadd-b arshadd-b changed the title Add v1beta2 changes to IBMPowerVSImage [WIP] Add v1beta2 changes to IBMPowerVSImage Jul 1, 2025
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jul 1, 2025
@arshadd-b arshadd-b force-pushed the update-powervs-image branch from 9632fae to a3756f6 Compare July 2, 2025 15:56
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 2, 2025
@arshadd-b arshadd-b force-pushed the update-powervs-image branch 3 times, most recently from 5d46687 to eaf2b42 Compare July 2, 2025 17:06
@arshadd-b arshadd-b force-pushed the update-powervs-image branch from eaf2b42 to 1be332d Compare July 7, 2025 06:00
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 7, 2025
@arshadd-b arshadd-b force-pushed the update-powervs-image branch from 0956e60 to 322a3dc Compare July 8, 2025 05:00
Copy link
Contributor

@Karthik-K-N Karthik-K-N left a 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.

@Amulyam24
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 11, 2025
@arshadd-b arshadd-b force-pushed the update-powervs-image branch from 75af036 to 7d51cd4 Compare July 15, 2025 04:27
@arshadd-b arshadd-b requested a review from Karthik-K-N July 18, 2025 07:25
@arshadd-b arshadd-b changed the title [WIP] Add v1beta2 changes to IBMPowerVSImage Add v1beta2 changes to IBMPowerVSImage Jul 18, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 18, 2025
@arshadd-b
Copy link
Contributor Author

Image queued
Screenshot 2025-07-16 at 7 15 54 PM

Image Importing
Screenshot 2025-07-16 at 7 17 20 PM

Image Ready
Screenshot 2025-07-16 at 7 14 51 PM

Copy link
Contributor

@Amulyam24 Amulyam24 left a 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

Comment on lines +393 to +394
infrav1.IBMPowerVSImageReadyCondition,
infrav1.IBMPowerVSImageReadyV1Beta2Condition,
Copy link
Contributor

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?

Copy link
Contributor Author

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will check this

Copy link
Contributor Author

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.

@arshadd-b arshadd-b force-pushed the update-powervs-image branch from 666478f to 7399a47 Compare July 23, 2025 04:45
@arshadd-b arshadd-b force-pushed the update-powervs-image branch from 7399a47 to 7ce6672 Compare July 24, 2025 03:13
@arshadd-b arshadd-b requested a review from Amulyam24 July 24, 2025 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/provider/ibmcloud Issues or PRs related to ibmcloud provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EPIC: Update CAPIBM resources to use v1beta2 API
4 participants