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: Error messages when Terraform validation fails due to bad credentials #1726

Merged
merged 2 commits into from
Mar 16, 2021

Conversation

aron
Copy link
Contributor

@aron aron commented Mar 15, 2021

  • Ready for review
  • Follows CONTRIBUTING rules
  • Reviewed by Snyk internal team

What does this PR do?

Fixes CC-751

Handles 401 authentication errors when making a validation request to Terraform, previously we would always return a result regardless of the HTTP response, this resulted in misleading error messages.

This PR also fixes the type definition of the request() function so the caller no longer has to unwrap the returned response from the promise. The return type for the non-callback approach is now Promise<{res: NeedleRes, body: any}> instead of Promise<void | {res: NeedleRes, body: any}>

Where should the reviewer start?

All the code is in src/lib/iac/iac-parser.ts, we have no unit tests for this code path :/ and it will all soon be replaced by the --experimental branch.

How should this be manually tested?

1. Invalidate the API token e.g. `snyk config set api=foo`
2. Test a Terraform file e.g `node ./dist/cli iac test ./test/fixtures/iac/terraform/sg_open_ssh.tf`

What are the relevant tickets?

https://snyksec.atlassian.net/browse/CC-751

Screenshots

Before

image

After
image

We now distinguish between the callback variant and the promise
@aron aron requested a review from a team as a code owner March 15, 2021 21:59
@aron aron requested a review from a team March 15, 2021 21:59
@aron aron requested a review from a team as a code owner March 15, 2021 21:59
@github-actions
Copy link
Contributor

github-actions bot commented Mar 15, 2021

Expected release notes (by @aron)

fixes:
Show auth error message when Terraform fails (ecb74d6)

others (will not be included in Semantic-Release notes):
Fix request type definition (b7f89aa)

  • I hereby acknowledge these release notes are 🥙 AWESOME 🥙

@github-actions
Copy link
Contributor

Warnings
⚠️ You've modified files in src/ directory, but haven't updated anything in test folder. Is there something that could be tested?

Generated by 🚫 dangerJS against ecb74d6

Copy link
Contributor

@ipapast ipapast left a comment

Choose a reason for hiding this comment

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

Ran it locally with invalid token. works with a nice message :)

@aron aron merged commit 5ed097d into master Mar 16, 2021
@aron aron deleted the fix/terraform-auth-CC-751 branch March 16, 2021 19:07
This was referenced Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects