-
Notifications
You must be signed in to change notification settings - Fork 569
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
Change iac gen-driftignore
to iac update-exclude-policy
#3046
Conversation
|
7d419b1
to
0844579
Compare
gen-driftignore
behavioriac gen-driftignore
to iac update-exclude-policy
0844579
to
17ee376
Compare
This PR modifies files linked to issues tracked in Stepsize. You might want to review their status, priority, and scope. We are losing types because of hotloading components in a JS file
Mention [stepsize] in a comment if you'd like to report some technical debt. See examples here. |
17ee376
to
81b89d4
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.
Looking good! This works in the case where there is already a .snyk
file present, but errors with ENOENT when it does not. snyk ignore
doesn't have this problem. We might want to touch the file first (unconditionally, idempotently).
Also, I noticed some stderr output from driftctl: "Hint: use gen-driftignore command to generate a .driftignore file based on your drifts". We might want to suppress / manipulate this before printing it for the user, to use this new command. WDYT?
81b89d4
to
bd65e5a
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.
👍 looks good to me.
bd65e5a
to
a3a907b
Compare
Fixed, I used the same logic than the ignore cmd.
Yeah we can probably either remove that sentence from the engine, or check if we are wrapped in the snyk CLI and hide it only in that case. |
Definitely agree with avoiding having anything snyk specific in the drift codebase. My understanding is that we want to aim for the drift -> snyk communication to be essentially machine to machine right? So perhaps we just need a flag or an environment variable to suppress "user info" output or help text. |
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.
LGTM 🥳
Yeah we can probably either remove that sentence from the engine
If that's an option, then I reckon we should do it. Less noise and all...
I'm not sure that the second approach (basically doing if (isWrappedBySnyk) { ... }) is really something that we want
IMO that's definitely not something we want, but it also isn't what I was suggesting! I was suggesting doing string replacement in this CLI on the stderr that it receives from driftctl. But if removing the message is an option, I prefer that one.
This commit rename the existing `iac gen-driftignore` command and also change its behavior. We are now generating our own exclude logic from the snyk CLI instead of calling driftctl binary to update the `.driftignore`.
a3a907b
to
9ac4cf1
Compare
Done here snyk/driftctl#1449 |
What does this PR do?
This PR move the responsibility to generate drift exclusion rule from driftctl to snyk CLI.
We are now generating our own rules in the snyk policy file instead of using driftctl to generate a
.driftignore
More context about that choice in the Jira ticket.
Where should the reviewer start?
src/cli/commands/update-exclude-policy.ts
How should this be manually tested?
$ cat test/fixtures/iac/drift/analysis.json | bin/snyk iac update-exclude-policy
Ensure your .snyk file is update with those new exclude rules
Any background context you want to provide?
The exclude logic is based on this code from driftctl
Changes in help files are just FYI here, changes are in a draft PR in gitbook
What are the relevant tickets?
https://snyksec.atlassian.net/browse/CFG-1669
Screenshots
Additional questions