Skip to content

Conversation

@sivchari
Copy link
Member

What this PR does / why we need it:

Go has errors package to use Is() and As(). These API can check recursively if the err is one we want.
So I use errors package to check recursively.

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 k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area PR is missing an area label size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 16, 2024
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

generally seems better to use Is() and As() instead of the type assertions.

@sbueringer sbueringer added this to the v1.8 milestone Jul 17, 2024
@sivchari
Copy link
Member Author

/assign

@sivchari sivchari changed the title 🐛 Use errors package of Go 🌱 Use errors package of Go Jul 23, 2024
@sbueringer sbueringer removed this from the v1.8 milestone Jul 25, 2024
@sbueringer sbueringer added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 16, 2024
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2024
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2024
@sbueringer
Copy link
Member

sbueringer commented Nov 14, 2024

I currently don't find the time to review this PR. Just one comment, we have to be really really sure this doesn't break anything accidentally (by catching more or less errors than before)

Copy link

@anmazzotti anmazzotti left a comment

Choose a reason for hiding this comment

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

Agree with @neolit123 a few instances could use errors.Is instead, but this is also ok.

Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

Let's use errors.As only if we actually want to use the error afterwards. In a lot of cases we only check but never use the error value

@sivchari
Copy link
Member Author

sivchari commented Feb 5, 2025

I rebased and fixed some review points. Thanks.

@sivchari
Copy link
Member Author

sivchari commented Feb 11, 2025

The list that I should write UnitTests

  • handleGithubErr
  • (g *gitHubRepository) getVersions
  • (g *gitHubRepository) handleGithubErr
  • (r *KubeadmControlPlaneReconciler) Reconcile

@sivchari sivchari force-pushed the use-errors-pkg branch 2 times, most recently from 4e9cb5f to 121d9fb Compare April 29, 2025 05:57
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 6, 2025
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 7, 2025
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

as i commented here, kubeadm wrapped the stdlib errors package to resemble pkg/errors in a consistent manner.

#6688 (comment)

that i think is the better solution instead of importing the stdlib errors and using fmt.Errorf in some places.

capi can fork the relevant bits from kubeadm. maybe we can put this code in k/component-base eventually but that needs agreement with other sigs.

@sivchari
Copy link
Member Author

sivchari commented Aug 7, 2025

It sounds good, then if we decide to use it, we might be able to drop the direct dependence of pkg/errors completely.

@sbueringer
Copy link
Member

sbueringer commented Aug 8, 2025

Can we please stick with using pkg/errors for this PR?

We can have the discussion on how to migrate and then make this a dedicated effort, but it should not be mixed with other changes in my opinion.

(For what it's worth, I agree with Lubomir's suggestion on how to migrate)

Signed-off-by: sivchari <[email protected]>
Signed-off-by: sivchari <[email protected]>
Signed-off-by: sivchari <[email protected]>
@sbueringer sbueringer added the area/misc Issues or PRs not related to any other area label Sep 15, 2025
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/needs-area PR is missing an area label label Sep 15, 2025
@sbueringer
Copy link
Member

Thx!

/lgtm
/approve
/test pull-cluster-api-e2e-main

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 15, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 2f3a32316c42371c579f7a6cece3686d37c59a49

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 15, 2025
@sbueringer
Copy link
Member

Will be fixed elsewhere

/override pull-cluster-api-e2e-main

@k8s-ci-robot
Copy link
Contributor

@sbueringer: Overrode contexts on behalf of sbueringer: pull-cluster-api-e2e-main

In response to this:

Will be fixed elsewhere

/override pull-cluster-api-e2e-main

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.

@k8s-ci-robot k8s-ci-robot merged commit a9fbe11 into kubernetes-sigs:main Sep 15, 2025
20 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.12 milestone Sep 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/misc Issues or PRs not related to any other area cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants