-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🌱 Use errors package of Go #10875
base: main
Are you sure you want to change the base?
🌱 Use errors package of Go #10875
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,6 +61,8 @@ var ( | |
retryableOperationTimeout = 1 * time.Minute | ||
) | ||
|
||
var rateLimitError github.RateLimitError | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can be a local var in getVersions()? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rateLimitError is used in many func of repository package. So I think it's better to prevent omission of change. |
||
|
||
// gitHubRepository provides support for providers hosted on GitHub. | ||
// | ||
// We support GitHub repositories that use the release feature to publish artifacts and versions. | ||
|
@@ -319,7 +321,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.As(listReleasesErr, &rateLimitError) { | ||
return false, retryError | ||
} | ||
return false, nil | ||
|
@@ -334,7 +336,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.As(listReleasesErr, &rateLimitError) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not use errors.Is(....,, RateLimitError)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code is originally type assertion, so errors.As is better than using errors.Is |
||
return false, retryError | ||
} | ||
return false, nil | ||
|
@@ -385,7 +387,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.As(getReleasesErr, &rateLimitError) { | ||
return false, retryError | ||
} | ||
return false, nil | ||
|
@@ -467,7 +469,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.As(downloadReleaseError, &rateLimitError) { | ||
return false, retryError | ||
} | ||
return false, nil | ||
|
@@ -500,10 +502,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 { | ||
if errors.As(err, &rateLimitError) { | ||
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") | ||
} | ||
if ghErr, ok := err.(*github.ErrorResponse); ok { | ||
|
||
var errorResponse github.ErrorResponse | ||
if errors.As(err, &errorResponse) { | ||
ghErr := err.(*github.ErrorResponse) | ||
if ghErr.Response.StatusCode == http.StatusNotFound { | ||
return errNotFound | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -341,14 +341,28 @@ type scope struct { | |
bootstrapConfig *unstructured.Unstructured | ||
} | ||
|
||
func is(err error) bool { | ||
errs := []error{ | ||
errNoControlPlaneNodes, errLastControlPlaneNode, | ||
errNilNodeRef, errClusterIsBeingDeleted, | ||
errControlPlaneIsBeingDeleted, | ||
} | ||
for _, e := range errs { | ||
if errors.Is(err, e) { | ||
return true | ||
} | ||
} | ||
return false | ||
} | ||
|
||
func (r *Reconciler) reconcileDelete(ctx context.Context, cluster *clusterv1.Cluster, m *clusterv1.Machine) (ctrl.Result, error) { //nolint:gocyclo | ||
log := ctrl.LoggerFrom(ctx) | ||
|
||
err := r.isDeleteNodeAllowed(ctx, cluster, m) | ||
isDeleteNodeAllowed := err == nil | ||
if err != nil { | ||
switch err { | ||
case errNoControlPlaneNodes, errLastControlPlaneNode, errNilNodeRef, errClusterIsBeingDeleted, errControlPlaneIsBeingDeleted: | ||
switch { | ||
case is(err): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isKnownError or something more descriptive, also you can inline the function inside reconcileDelete |
||
nodeName := "" | ||
if m.Status.NodeRef != nil { | ||
nodeName = m.Status.NodeRef.Name | ||
|
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 not use errors.Is(....,, RateLimitError)?
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.
Same reason, thanks.