Skip to content

Conversation

@alexander-demicev
Copy link
Contributor

What this PR does / why we need it:
Part of #12291

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 #

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area PR is missing an area label labels Oct 8, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign sbueringer 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 the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 8, 2025
@sbueringer sbueringer added the area/machine Issues or PRs related to machine lifecycle management label Oct 8, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/needs-area PR is missing an area label label Oct 8, 2025
@alexander-demicev alexander-demicev force-pushed the machinecontroller branch 2 times, most recently from a88faa8 to ed1835e Compare October 15, 2025 10:41
@alexander-demicev
Copy link
Contributor Author

@sbueringer all comments addressed

@sbueringer
Copy link
Member

I think you can go ahead and add unit test coverage

// Support for multiple extensions will be introduced in a future iteration.
extensions, err := r.RuntimeClient.GetAllExtensions(ctx, runtimehooksv1.UpdateMachine, s.machine)
if err != nil {
return ctrl.Result{}, "", errors.Wrap(err, "failed to get registered UpdateMachine extensions")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return ctrl.Result{}, "", errors.Wrap(err, "failed to get registered UpdateMachine extensions")
return ctrl.Result{}, "", err

I think the error build inside of GetAllExtensions already has enough context, this would just repeat it a bit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/machine Issues or PRs related to machine lifecycle management cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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.

4 participants