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

[CFG-1660] new ui for snyk iac describe #3103

Merged
merged 2 commits into from
Apr 15, 2022
Merged

Conversation

moadibfr
Copy link

@moadibfr moadibfr commented Apr 6, 2022

What does this PR do?

This PR update the snyk iac describe output with new ui by the design team.
(https://miro.com/app/board/uXjVOVYR5zg=/?share_link_id=306344151216)
This is a first iteration for the ui.

Where should the reviewer start?

Sorry about that but I made some refactoring to not have everything inside a single big typescript file, I hope this won't make the reviewing too painfull.
The pr introduce some change in drift.ts for the logic about driftctl first run output handling (mainly to use driftctl fmt only for html output).
Main addition will then be inside drift/output.ts.

What are the relevant tickets?

https://snyksec.atlassian.net/browse/CFG-1660

Screenshots

image

@moadibfr moadibfr force-pushed the feat/better_describe_ux branch 2 times, most recently from 44fbe39 to 033cfce Compare April 6, 2022 17:14
@moadibfr moadibfr force-pushed the feat/better_describe_ux branch 4 times, most recently from d645c74 to a220f3f Compare April 8, 2022 09:21
@moadibfr moadibfr changed the title feat: change output for describe command [CFG-1660] new ui for snyk iac describe Apr 8, 2022
@moadibfr moadibfr force-pushed the feat/better_describe_ux branch 2 times, most recently from 459e20e to 6bff2bc Compare April 8, 2022 12:59
@moadibfr moadibfr marked this pull request as ready for review April 8, 2022 13:13
@moadibfr moadibfr requested review from a team as code owners April 8, 2022 13:13
@moadibfr moadibfr force-pushed the feat/better_describe_ux branch from 6bff2bc to 2d404d6 Compare April 8, 2022 14:14
@wbeuil wbeuil force-pushed the feat/better_describe_ux branch 2 times, most recently from 19bf6dc to b0f11d0 Compare April 13, 2022 14:07
@wbeuil wbeuil requested review from craigfurman and removed request for sundowndev-snyk April 13, 2022 14:25
@wbeuil wbeuil force-pushed the feat/better_describe_ux branch from b0f11d0 to a34e33b Compare April 13, 2022 15:32
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.

Full disclosure: this isn't the deepest review I've ever given. It's not a rubber stamp either - basically, all I've done is test this out, and then scan through the overall code structure and tests.

This was a bit tricky to review due to the combined refactor (a good refactor though), and functional changes. Ideally these would be in different commits. But I know 2 different people worked on it at different times, so no big deal.

Overall, LGTM! But perhaps get one more set of eyeballs on this, it's a large patch.

Non-blocking suggestion that's probably better for a follow-on: instead of having lib/iac/drift.ts and lib/iac/drift/ separately, we could have a single directory lib/iac/drift/, with a thin public interface in index.ts. I'm no TS expert, but AFAIK index.ts is a special case that can be imported by the name of its parent directory.

@wbeuil
Copy link
Contributor

wbeuil commented Apr 14, 2022

Non-blocking suggestion that's probably better for a follow-on: instead of having lib/iac/drift.ts and lib/iac/drift/ separately, we could have a single directory lib/iac/drift/, with a thin public interface in index.ts. I'm no TS expert, but AFAIK index.ts is a special case that can be imported by the name of its parent directory.

That's a pretty good observation here Craig, I didn't think of this one and I 100% agree with you.

package.json Show resolved Hide resolved
@wbeuil wbeuil requested a review from a user April 14, 2022 15:40
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Approving as there's not much choice around jsondiffpatch. Please be aware though that if jsondiffpatch or its dependencies have vulns, IaC will need to find an alternative or fork the project to resolve it as an incident. It might be worth IaC forking it now, publishing a fork, and using it here to avoid any potential malicious releases in the future.

@wbeuil wbeuil merged commit 1f11565 into master Apr 15, 2022
@wbeuil wbeuil deleted the feat/better_describe_ux branch April 15, 2022 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants