-
Notifications
You must be signed in to change notification settings - Fork 564
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: create only one project group per execution #3262
Conversation
fa5fee4
to
9c997ab
Compare
34ca9ef
to
2ea40d3
Compare
2ea40d3
to
6f0c180
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had a chat with @francescomari and Esra regarding the expected design and also some edge cases that fail, such as snyk iac test . .. --report
.
Francesco is making some changes and we are going to review again
6f0c180
to
ed48bcc
Compare
@ipapast I added a new error class in |
@francescomari I tested different scenarios we discussed on the call, including snyk iac . .. --report and they all look good now. But I also tried out something more weird, like:
You will see in the last line a project name appearing as 'var_deref" which is not true. When I check the platform everything is correctly grouped under "snyk" |
20c6679
to
73d30c6
Compare
@ipapast that was a really good catch. It should be fixed at 73d30c66, according to the unit tests. Let's see what the acceptance tests say. |
73d30c6
to
12ee279
Compare
5502bf1
to
636e3cd
Compare
ebb8065
to
08e4038
Compare
Given the latest conversations about this feature, the scope of this PR changed. Instead of replacing the current implementation for processing results, the PR adds a new implementation (single project group) in The code in I could have been more granular when splitting the two implementations, but I decided not to at the cost of some code duplication. When the new implementation will become the default, removing the old one will be as easy as deleting the Some tests are missing for |
08e4038
to
fd3c0ce
Compare
What does this PR do?
This PR changes the behaviour of the
iac test
command when multiple paths are passed on the command line. This PR changes the behaviour of the--report
flag, too. The implemented behaviour is the following:iac test
recursively scans the current working directory by default.--report
flag is used, theiac test
command creates one project group in the platform named after the name of the current working directory.Where should the reviewer start?
This PR extracts the scan logic into src/cli/commands/test/iac/scan.ts. You can start the review fro there.
How should this be manually tested?
Move anywhere on the file system and run
snyk iac test ..
. The command will fail with an error, because the only provided path is outside of the current working directory.Move in the root of this repository and run
snyk iac test --report
. This command creates only one project group in the platform, named after this repository. All the project names will be the paths of the IaC files in this repository, relative from the current working directory.Move into the test/fixtures/iac directory and run
snyk iac test --report
. The command creates only one project group namediac
, because the current working directory is not a Git repository. All the project names are relative from the current working directory.Any background context you want to provide?
This behaviour has been discussed in this document. The document contains additional links to interesting conversations that shaped the implementation in this PR.
What are the relevant tickets?
CFG-1840