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: Skip specific errors when parsing yaml IaC files #2003

Merged
merged 1 commit into from
Jun 9, 2021

Conversation

ipapast
Copy link
Contributor

@ipapast ipapast commented Jun 4, 2021

What does this PR do?

We identified a few cases that our services parse the yaml files in a different way. The node 'yaml' library is too 'strict', returning errors for validation of the YAML as well.

In this PR we focus on two specific errors: bad indentation and duplicate keys.
These two examples are not consistent with what Policy Engine returns so in this commit we intentionally skip errors for these cases, and return the parsed YAML document instead.

This will improve the consistency between the two parsers.

Where should the reviewer start?

The changes are in file-parser.ts and iac-parser.ts where we check if the error is one of the two error strings.

How should this be manually tested?

  • Create a file with a SemanticError, e.g. duplicate keys:
apiVersion: v1
kind: Pod
metadata:
  name: myapp-pod
spec:
  containers:
  something: here
metadata:
  another: thing

Run snyk-dev iac test duplicate_keys.yaml
See that it is no longer failing but showing results (if any).

What are the relevant tickets?

CC-927

Screenshots

Before:
image

After:
image

Additional questions

@ipapast ipapast requested a review from a team June 4, 2021 12:25
@ipapast ipapast force-pushed the fix/ignore-yaml-semantic-errors-iac branch 3 times, most recently from 251e6c2 to 7ecceac Compare June 8, 2021 20:17
@ipapast ipapast changed the title fix: Fail only on SyntaxYAMLError when parsing yaml IaC files fix: Skip specific errors when parsing yaml IaC files Jun 8, 2021
@ipapast ipapast force-pushed the fix/ignore-yaml-semantic-errors-iac branch from 15f9ce8 to e4de2fb Compare June 9, 2021 15:10
@ipapast
Copy link
Contributor Author

ipapast commented Jun 9, 2021

@almog27 @teodora-sandu I extracted the check as a function and gave it a descriptive name, also added an extra test. If you could have a look again 🙇

@ipapast ipapast force-pushed the fix/ignore-yaml-semantic-errors-iac branch 2 times, most recently from 8a148f0 to e20ad5a Compare June 9, 2021 15:21
@ipapast ipapast force-pushed the fix/ignore-yaml-semantic-errors-iac branch from e20ad5a to dc8ffa0 Compare June 9, 2021 15:52
The new yaml parser library does some validations that we do not do in Policy Engine, so we return less results in the CLI. These are not critical errors, so we decided to skip them and return the parsed file for consistency with the Policy Engine.
@ipapast ipapast force-pushed the fix/ignore-yaml-semantic-errors-iac branch from dc8ffa0 to 49c184d Compare June 9, 2021 15:57
@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2021

Expected release notes (by @ipapast)

fixes:
Skip specific errors when parsing yaml IaC files (49c184d)

others (will not be included in Semantic-Release notes):
get rid of changes from #1974 to fix broken test (217d1fa)
get npm to grab token directly from env (dcac487)
enforce prettier across entire project (6cb8953)
enforce eslint across entire project (d2aa5f5)
migrate to tempfile to tempy (5af6fdb)
upgrade to uuid@8 (1c65dd9)
let npm fetch the npm_token for us (12052e8)

  • I hereby acknowledge these release notes are 🥙 AWESOME 🥙

@ipapast ipapast self-assigned this Jun 9, 2021
@ipapast ipapast merged commit 0236fa8 into master Jun 9, 2021
@ipapast ipapast deleted the fix/ignore-yaml-semantic-errors-iac branch June 9, 2021 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants