Skip to content

🌱 Use errors package of Go #10875

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 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -194,11 +194,6 @@ func (r *KubeadmConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques
// Lookup the cluster the config owner is associated with
cluster, err := util.GetClusterByName(ctx, r.Client, configOwner.GetNamespace(), configOwner.ClusterName())
if err != nil {
if errors.Cause(err) == util.ErrNoCluster {
log.Info(fmt.Sprintf("%s does not belong to a cluster yet, waiting until it's part of a cluster", configOwner.GetKind()))
return ctrl.Result{}, nil
}

if apierrors.IsNotFound(err) {
log.Info("Cluster does not exist yet, waiting until it is created")
return ctrl.Result{}, nil
Expand Down
6 changes: 4 additions & 2 deletions cmd/clusterctl/client/cluster/template.go
Original file line number Diff line number Diff line change
Expand Up @@ -303,10 +303,12 @@ func getGitHubClient(ctx context.Context, configVariablesClient config.Variables
return github.NewClient(authenticatingHTTPClient), nil
}

var errRateLimit = errors.New("rate limit for github api has been reached. Please wait one hour or get a personal API token and assign it to the GITHUB_TOKEN environment variable")

// handleGithubErr wraps error messages.
func handleGithubErr(err error, message string, args ...interface{}) error {
if _, ok := err.(*github.RateLimitError); ok {
return errors.New("rate limit for github api has been reached. Please wait one hour or get a personal API token and assign it to the GITHUB_TOKEN environment variable")
return errRateLimit
}
return errors.Wrapf(err, message, args...)
return fmt.Errorf("%s: %w", fmt.Sprintf(message, args...), err)
}
38 changes: 38 additions & 0 deletions cmd/clusterctl/client/cluster/template_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (

"github.com/google/go-github/v53/github"
. "github.com/onsi/gomega"
"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

Expand Down Expand Up @@ -517,6 +518,43 @@ func Test_templateClient_GetFromURL(t *testing.T) {
}
}

func Test_handleGithubErr(t *testing.T) {
tests := []struct {
name string
err error
message string
args []any
want error
}{
{
name: "Return error",
err: errors.New("error"),
message: "message %s and %s",
args: []any{"arg1", "arg2"},
want: fmt.Errorf("message arg1 and arg2: %w", errors.New("error")),
},
{
name: "Return RateLimitError",
err: &github.RateLimitError{
Response: &http.Response{
StatusCode: http.StatusForbidden,
},
},
message: "",
args: nil,
want: errRateLimit,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)

got := handleGithubErr(tt.err, tt.message, tt.args...)
g.Expect(got.Error()).To(Equal(tt.want.Error()))
})
}
}

func mustParseURL(rawURL string) *url.URL {
rURL, err := url.Parse(rawURL)
if err != nil {
Expand Down
24 changes: 13 additions & 11 deletions cmd/clusterctl/client/repository/repository_github.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ const (
)

var (
errNotFound = errors.New("404 Not Found")
errNotFound = errors.New("404 Not Found")
errRateLimit = errors.New("rate limit for github api has been reached. Please wait one hour or get a personal API token and assign it to the GITHUB_TOKEN environment variable")

// Caches used to limit the number of GitHub API calls.

Expand Down Expand Up @@ -319,7 +320,7 @@ func (g *gitHubRepository) getVersions(ctx context.Context) ([]string, error) {
if listReleasesErr != nil {
retryError = g.handleGithubErr(listReleasesErr, "failed to get the list of releases")
// Return immediately if we are rate limited.
if _, ok := listReleasesErr.(*github.RateLimitError); ok {
if errors.Is(retryError, errRateLimit) {
return false, retryError
}
return false, nil
Expand All @@ -334,7 +335,7 @@ func (g *gitHubRepository) getVersions(ctx context.Context) ([]string, error) {
if listReleasesErr != nil {
retryError = g.handleGithubErr(listReleasesErr, "failed to get the list of releases")
// Return immediately if we are rate limited.
if _, ok := listReleasesErr.(*github.RateLimitError); ok {
if errors.Is(retryError, errRateLimit) {
return false, retryError
}
return false, nil
Expand Down Expand Up @@ -384,7 +385,7 @@ func (g *gitHubRepository) getReleaseByTag(ctx context.Context, tag string) (*gi
return false, retryError
}
// Return immediately if we are rate limited.
if _, ok := getReleasesErr.(*github.RateLimitError); ok {
if errors.Is(retryError, errRateLimit) {
return false, retryError
}
return false, nil
Expand Down Expand Up @@ -466,7 +467,7 @@ func (g *gitHubRepository) downloadFilesFromRelease(ctx context.Context, release
if downloadReleaseError != nil {
retryError = g.handleGithubErr(downloadReleaseError, "failed to download file %q from %q release", *release.TagName, fileName)
// Return immediately if we are rate limited.
if _, ok := downloadReleaseError.(*github.RateLimitError); ok {
if errors.Is(retryError, errRateLimit) {
return false, retryError
}
return false, nil
Expand Down Expand Up @@ -500,12 +501,13 @@ func (g *gitHubRepository) downloadFilesFromRelease(ctx context.Context, release
// handleGithubErr wraps error messages.
func (g *gitHubRepository) handleGithubErr(err error, message string, args ...interface{}) error {
if _, ok := err.(*github.RateLimitError); ok {
return errors.New("rate limit for github api has been reached. Please wait one hour or get a personal API token and assign it to the GITHUB_TOKEN environment variable")
return errRateLimit
}
if ghErr, ok := err.(*github.ErrorResponse); ok {
if ghErr.Response.StatusCode == http.StatusNotFound {
return errNotFound
}

var ghErr *github.ErrorResponse
if errors.As(err, &ghErr) && ghErr.Response.StatusCode == http.StatusNotFound {
return errNotFound
}
return errors.Wrapf(err, message, args...)

return fmt.Errorf("%s: %w", fmt.Sprintf(message, args...), err)
}
52 changes: 52 additions & 0 deletions cmd/clusterctl/client/repository/repository_github_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (

"github.com/google/go-github/v53/github"
. "github.com/onsi/gomega"
"github.com/pkg/errors"
"k8s.io/utils/ptr"

clusterctlv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3"
Expand Down Expand Up @@ -1108,3 +1109,54 @@ func Test_gitHubRepository_releaseNotFound(t *testing.T) {
})
}
}

func Test_handleGithubErr(t *testing.T) {
tests := []struct {
name string
err error
message string
args []any
want error
}{
{
name: "Return error",
err: errors.New("error"),
message: "message %s and %s",
args: []any{"arg1", "arg2"},
want: fmt.Errorf("message arg1 and arg2: %w", errors.New("error")),
},
{
name: "Return RateLimitError",
err: &github.RateLimitError{
Response: &http.Response{
StatusCode: http.StatusForbidden,
},
},
message: "",
args: nil,
want: errRateLimit,
},
{
name: "Return ErrorResponse",
err: &github.ErrorResponse{
Response: &http.Response{
StatusCode: http.StatusNotFound,
},
},
message: "",
args: nil,
want: errNotFound,
},
}

gRepo := &gitHubRepository{}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)

got := gRepo.handleGithubErr(tt.err, tt.message, tt.args...)
g.Expect(got.Error()).To(Equal(tt.want.Error()))
})
}
}
3 changes: 1 addition & 2 deletions controlplane/kubeadm/internal/controllers/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import (
"strings"
"time"

"github.com/pkg/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/utils/ptr"
Expand Down Expand Up @@ -100,7 +99,7 @@ func (r *KubeadmControlPlaneReconciler) updateStatus(ctx context.Context, contro

workloadCluster, err := controlPlane.GetWorkloadCluster(ctx)
if err != nil {
return errors.Wrap(err, "failed to create remote cluster client")
return fmt.Errorf("failed to create remote cluster client: %w", err)
}
status, err := workloadCluster.ClusterStatus(ctx)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion exp/internal/controllers/machinepool_controller_noderef.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ func (r *MachinePoolReconciler) reconcileNodeRefs(ctx context.Context, s *scope)
// Get the Node references.
nodeRefsResult, err := r.getNodeReferences(ctx, mp.Spec.ProviderIDList, mp.Spec.MinReadySeconds, s.nodeRefMap)
if err != nil {
if err == errNoAvailableNodes {
if errors.Is(err, errNoAvailableNodes) {
log.Info("Cannot assign NodeRefs to MachinePool, no matching Nodes")
// No need to requeue here. Nodes emit an event that triggers reconciliation.
return ctrl.Result{}, nil
Expand Down
2 changes: 1 addition & 1 deletion exp/internal/controllers/machinepool_controller_phases.go
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ func (r *MachinePoolReconciler) reconcileInfrastructure(ctx context.Context, s *
// Get and set Status.Replicas from the infrastructure provider.
err = util.UnstructuredUnmarshalField(infraConfig, &mp.Status.Replicas, "status", "replicas")
if err != nil {
if err != util.ErrUnstructuredFieldNotFound {
if !errors.Is(err, util.ErrUnstructuredFieldNotFound) {
return ctrl.Result{}, errors.Wrapf(err, "failed to retrieve replicas from infrastructure provider for MachinePool %q in namespace %q", mp.Name, mp.Namespace)
}
}
Expand Down
2 changes: 1 addition & 1 deletion internal/controllers/cluster/cluster_controller_phases.go
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,7 @@ func (r *Reconciler) reconcileKubeconfig(ctx context.Context, s *scope) (ctrl.Re
switch {
case apierrors.IsNotFound(err):
if err := kubeconfig.CreateSecret(ctx, r.Client, cluster); err != nil {
if err == kubeconfig.ErrDependentCertificateNotFound {
if errors.Is(err, kubeconfig.ErrDependentCertificateNotFound) {
log.Info("Could not find secret for cluster, requeuing", "Secret", secret.ClusterCA)
return ctrl.Result{RequeueAfter: 30 * time.Second}, nil
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ func (r *Reconciler) ApplyClusterResourceSet(ctx context.Context, cluster *clust
for i, resource := range clusterResourceSet.Spec.Resources {
unstructuredObj, err := r.getResource(ctx, resource, cluster.GetNamespace())
if err != nil {
if err == ErrSecretTypeNotSupported {
if errors.Is(err, ErrSecretTypeNotSupported) {
v1beta1conditions.MarkFalse(clusterResourceSet, addonsv1.ResourcesAppliedV1Beta1Condition, addonsv1.WrongSecretTypeV1Beta1Reason, clusterv1.ConditionSeverityWarning, err.Error())
conditions.Set(clusterResourceSet, metav1.Condition{
Type: addonsv1.ClusterResourceSetResourcesAppliedCondition,
Expand Down
2 changes: 1 addition & 1 deletion internal/controllers/machine/drain/drain.go
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ evictionLoop:
// Ensure the causes are also included in the error message.
// Before: "Cannot evict pod as it would violate the pod's disruption budget."
// After: "Cannot evict pod as it would violate the pod's disruption budget. The disruption budget nginx needs 20 healthy pods and has 20 currently"
if ok := errors.As(err, &statusError); ok {
if errors.As(err, &statusError) {
errorMessage := statusError.Status().Message
if statusError.Status().Details != nil {
var causes []string
Expand Down
3 changes: 1 addition & 2 deletions internal/controllers/machine/machine_controller_noderef.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,11 @@ func (r *Reconciler) reconcileNode(ctx context.Context, s *scope) (ctrl.Result,
// Even if Status.NodeRef exists, continue to do the following checks to make sure Node is healthy
node, err := r.getNode(ctx, remoteClient, *machine.Spec.ProviderID)
if err != nil {
if err == ErrNodeNotFound {
if errors.Is(err, ErrNodeNotFound) {
if !s.machine.DeletionTimestamp.IsZero() {
// Tolerate node not found when the machine is being deleted.
return ctrl.Result{}, nil
}

// While a NodeRef is set in the status, failing to get that node means the node is deleted.
// If Status.NodeRef is not set before, node still can be in the provisioning state.
if machine.Status.NodeRef != nil {
Expand Down