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

fix: handle errors from /share-results [CFG-2028] #3454

Merged
merged 1 commit into from
Jul 13, 2022

Conversation

ipapast
Copy link
Contributor

@ipapast ipapast commented Jul 12, 2022

What does this PR do?

While investigating https://snyk.zendesk.com/agent/tickets/26894, we noticed that client errors (e.g. HTTP 422) from the IaC share-results endpoint are not communicated to the CLI caller at all.

Example error returned in prod-registry that is not surfaced to users (example with non english chars in tag, but this can be caused by any non Az09 characters):

snyk iac test test/fixtures/iac/terraform/var_deref/nested_var_deref/ --project-tags=client=nαδαδσδαomnom,Team=Cloud --report

InvalidUserInputError: invalid tag value: contains invalid characters
    at assertValidValue (/srv/app/dist/lib/tags/validation.js:36:15)
    at assertValidFields (/srv/app/dist/lib/tags/validation.js:50:34)
    at parse (/srv/app/dist/web/routes/project/tags/parse.js:17:38)
    at /srv/app/dist/web/routes/api/v1/iac-cli-share-results.js:36:39
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)

Users should see the error in the CLI’s output so that they know they need to do something in order to record project results, otherwise the problem might go un-noticed for some time. The silence can also lead users to believe that it is a backend error, and not an input error.

We will not print the specific validation error as part of this PR - this code is to be refactored in the next weeks so we will re-write this again.

This PR updates the call to share-results to handle all 500 errors (that's what we wrap them with in Registry).

before - the error was silent for the invalid tag:
image

after

image

@ipapast ipapast marked this pull request as ready for review July 12, 2022 11:36
@ipapast ipapast requested a review from a team as a code owner July 12, 2022 11:36
@ipapast ipapast requested review from teodora-sandu, francescomari and YairZ101 and removed request for teodora-sandu July 12, 2022 11:36
Copy link
Contributor

@YairZ101 YairZ101 left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this :)

@ipapast ipapast requested a review from YairZ101 July 12, 2022 11:44
@ipapast
Copy link
Contributor Author

ipapast commented Jul 12, 2022

Thank you for fixing this :)

no problem. could you have another look, just added a 2nd commit, I think we should be throwing an error for anything non 200ish - like this: 44381fc

what do you think?

@YairZ101
Copy link
Contributor

I think we should be throwing an error for anything non 200ish - like this: 44381fc

what do you think?

sounds like a good idea

@ipapast ipapast force-pushed the fix/error-for-share-results branch 3 times, most recently from d160c84 to f969812 Compare July 12, 2022 11:53
@ipapast ipapast force-pushed the fix/error-for-share-results branch from f969812 to 5871079 Compare July 12, 2022 15:18
@ipapast ipapast requested review from a team as code owners July 12, 2022 15:18
@ipapast ipapast changed the title fix: handle errors from /share-results fix: handle errors from /share-results [CFG-2028] Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants