-
Notifications
You must be signed in to change notification settings - Fork 26
Check for response code 200, if not 200 return error #354
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
base: main
Are you sure you want to change the base?
Conversation
Reviewer's GuideAdd an HTTP 200 status check to early-detect non-successful responses and refine the download error message to include the request URL. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @cwiggs - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `pkg/engine/config.go:190` </location>
<code_context>
defer resp.Body.Close()
+
+ if resp.StatusCode != http.StatusOK {
+ return false, fmt.Errorf("received non-200 HTTP status: %d %s", resp.StatusCode, resp.Status)
+ }
+
</code_context>
<issue_to_address>
Error message could include the URL for better traceability.
Adding the URL will make it easier to identify which endpoint caused the error during debugging.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
return false, fmt.Errorf("received non-200 HTTP status: %d %s", resp.StatusCode, resp.Status)
=======
return false, fmt.Errorf("received non-200 HTTP status from %s: %d %s", urlStr, resp.StatusCode, resp.Status)
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
|
For what it’s worth I’ve used fetchit with both Bitbucket and Gitea so it’s not limited to only GitHub. What authentication method are you trying to use? I generally use ssh |
Looking at the code I believe it will work if you are using a username and password but not a PAT. |
I was trying to get fetchit working with a private gitlab repo and had to read the code to find out that it only seems to work with github? Adding a simple check like this will help users identify if there is an issue with the http request.
I plan to send future PRs so that fetchit will work with gitlab as well.
Thanks!
Summary by Sourcery
Add an HTTP status code check to downloadUpdateConfigFile to error on non-200 responses and improve error messaging with URL context.
Bug Fixes:
Enhancements: