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

refactor: replace cc parser with split functions [CC-1021] #2167

Merged
merged 1 commit into from Aug 17, 2021

Conversation

almog27
Copy link

@almog27 almog27 commented Aug 16, 2021

What does this PR do?

This PR updates the version of the cloud-config-parser dependency to use a newer version that takes care of a performance issue.
Instead of calculating the treeMap for each line in the file, we are calculating the treeMap once for each file, and sending it to the parser to get the correct line number.
Therefore, for a file that has multiple issues, we only calculate the treeMap once.

Where should the reviewer start?

How should this be manually tested?

  1. Download the new branch and install dependencies
  2. Run snyk iac test with --json or --sarif flags.
  3. See the line number returns in the response

What are the relevant tickets?

Jira Ticket CC-1021

Snapshots

Screen Shot 2021-08-16 at 11 37 49

@almog27 almog27 requested a review from a team August 16, 2021 08:38
@almog27 almog27 requested review from a team as code owners August 16, 2021 08:38
@almog27 almog27 self-assigned this Aug 16, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Aug 16, 2021

Messages
📖

This PR will not trigger a new version. It doesn't include any commit message with feat or fix.

Generated by 🚫 dangerJS against 1ed7d11

@almog27 almog27 requested review from teodora-sandu and removed request for p15r August 16, 2021 08:40
@almog27 almog27 force-pushed the refactor/replace-cc-parser-with-split-functions branch from 08a6088 to 8b65497 Compare August 16, 2021 10:32
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.

Looks good. Tests are breaking though, you will need to update the mocking

@almog27 almog27 force-pushed the refactor/replace-cc-parser-with-split-functions branch from 8b65497 to c8135f2 Compare August 16, 2021 13:17
Copy link
Contributor

@teodora-sandu teodora-sandu left a comment

Choose a reason for hiding this comment

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

Looks good, I asked for a few small changes

@ipapast ipapast changed the title refactor: replace cc parser with split functions refactor: replace cc parser with split functions [CC-1021] Aug 16, 2021
@almog27 almog27 force-pushed the refactor/replace-cc-parser-with-split-functions branch from ab88d74 to 86aee3d Compare August 17, 2021 08:14
@almog27 almog27 force-pushed the refactor/replace-cc-parser-with-split-functions branch from 86aee3d to c200e4c Compare August 17, 2021 08:52
@almog27 almog27 force-pushed the refactor/replace-cc-parser-with-split-functions branch from c200e4c to 1ed7d11 Compare August 17, 2021 09:27
@@ -147,7 +147,7 @@
"devDependencies": {
"@types/cross-spawn": "^6.0.2",
"@types/fs-extra": "^9.0.11",
"@types/jest": "^26.0.20",
"@types/jest": "^27.0.1",
Copy link
Author

Choose a reason for hiding this comment

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

This dependency was updated due to failure in regression tests due to the fact that the jest dependency is a newer version than the types

@almog27 almog27 merged commit f50bca7 into master Aug 17, 2021
@almog27 almog27 deleted the refactor/replace-cc-parser-with-split-functions branch August 17, 2021 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants