Skip to content
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

Display specific error messages when cloud-config fails #630

Merged
merged 1 commit into from
Jun 4, 2021

Conversation

rfratto
Copy link
Member

@rfratto rfratto commented Jun 3, 2021

PR Description

Changes the cloud-config command to attempt parsing the response body before returning an error that the response code was unexpected.

Which issue(s) this PR fixes

Fixes #360
Closes #629 (dupe of #360)

Notes to the Reviewer

PR Checklist

  • CHANGELOG updated
  • Documentation added
  • Tests updated

@rfratto rfratto merged commit 62a36fe into grafana:main Jun 4, 2021
@rfratto rfratto deleted the show-cloud-config-errors branch June 4, 2021 17:49
@bigpresh
Copy link

bigpresh commented Jun 4, 2021

Am I being blind - I don't see where you're actually outputting any extra info here?

The return "", fmt.Errorf("unexpected status code %d", resp.StatusCode) looks like the same as before; did you mean to add something from the parsed response into it? Or would the addition of dec.SetStrict(true) cause it to omit useful info (presumably only if the problem is that the response didn't contain valid YAML, but not if it did?

I'm not a Go coder, and I'm not very familiar with the Grafana Agent, so I may well be completely missing something obvious here.

I've tested it, but I'm getting the same result I did before - just could not retrieve agent cloud config: unexpected status code 500

I realised just after hitting submit on that comment that, whilst I'm pointing curl at the install script from the main branch, it is of course still grabbing the exact same deb package which won't have this change in - duh!

@rfratto
Copy link
Member Author

rfratto commented Jun 4, 2021

I don't see where you're actually outputting any extra info here?

It's (unfortunately) hidden from the diff but the extra info gets printed out a few lines below. That logic was always there, but was being cut out by me prematurely reading the response code.

The new flow here is:

  1. Try to parse the response as JSON.
  2. If parsing fails, and the result wasn't 200, return the status code as a best-effort error message. Otherwise, return the failure about reading the response body. This shouldn't happen in practice.
  3. If parsing succeeds, and the response JSON represents an error message, return that underlying error message, which is the part that was being hidden before. (If the response JSON doesn't represent an error message, use the success payload as the config file, returning successfully)

Once we do a release (sometime next week), the new error message will be could not retrieve agent cloud config: request was not successful: <issue from server>. Until then, if you're willing to hack around with things, you can manually run agentctl from the main branch :)

@bigpresh
Copy link

bigpresh commented Jun 4, 2021

Ohhh, I see - yeah, that makes sense now, thanks for the explanation!

I'll have a crack at building from the main branch and seeing if it gets me any more info.

@mattdurham mattdurham mentioned this pull request Sep 7, 2021
3 tasks
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Apr 13, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
3 participants