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

feat: replace YAML parser with library call [CC-1012] #2260

Merged
merged 1 commit into from
Sep 22, 2021

Conversation

teodora-sandu
Copy link
Contributor

What does this PR do?

This PR replaces the yaml library with a call to @snyk/cloud-config-parser, where we have moved our customised YAML parsing logic. This way when we have parsing issues that we need to fix, we can fix them in just one place.

How should this be manually tested?

Run:

  1. npm run build
  2. snyk-dev iac test ./test/fixtures/iac/kubernetes/pod-privileged.yaml
  3. snyk-dev iac test ./test/fixtures/iac/kubernetes/pod-valid.json

What are the relevant tickets?

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

Screenshots

Screenshot 2021-09-16 at 16 45 45

Screenshot 2021-09-16 at 16 45 49

@github-actions
Copy link
Contributor

github-actions bot commented Sep 16, 2021

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 0d75edc

@teodora-sandu teodora-sandu force-pushed the feat/replace-yaml-parser branch 2 times, most recently from 9b1b564 to b473e27 Compare September 17, 2021 06:42
@teodora-sandu teodora-sandu marked this pull request as ready for review September 17, 2021 07:38
@teodora-sandu teodora-sandu requested a review from a team September 17, 2021 07:38
@teodora-sandu teodora-sandu requested review from a team as code owners September 17, 2021 07:38
@teodora-sandu teodora-sandu requested review from craigfurman, tonidevine1, ipapast and rontalx and removed request for craigfurman and ipapast September 17, 2021 07:38
Copy link

@craigfurman craigfurman left a comment

Choose a reason for hiding this comment

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

Nice! Reviewer note: I haven't read the cloud config parser code again, I'm relying on our tests in both of these repos catching any regressions.

That is a really massive diff to the package-lock though, are we sure that's done the right thing?

@teodora-sandu teodora-sandu removed the request for review from rontalx September 17, 2021 11:01
@teodora-sandu teodora-sandu force-pushed the feat/replace-yaml-parser branch 4 times, most recently from 00b9fe2 to e8ff8ae Compare September 17, 2021 11:25
@teodora-sandu
Copy link
Contributor Author

Good shout on the package-lock.json. I followed the instructions in this thread to fix it: https://snyk.slack.com/archives/C1NREQSG0/p1631814339032800

Copy link
Member

@anthogez anthogez left a comment

Choose a reason for hiding this comment

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

maybe remove the ^ symbol from "@snyk/cloud-config-parser": "^1.11.0" to avoid breaking changes unless it's a desired behavior from your team

Copy link
Contributor

@jan-stehlik jan-stehlik left a comment

Choose a reason for hiding this comment

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

nice one @teodora-sandu and amazing PR description 🔥

package-lock.json Outdated Show resolved Hide resolved
@teodora-sandu teodora-sandu merged commit a7838da into master Sep 22, 2021
@teodora-sandu teodora-sandu deleted the feat/replace-yaml-parser branch September 22, 2021 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants