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.

Sign up for GitHub

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

Jump to bottom

feat(outputs): allow to set multiple outputs (#648) #1346

Merged
merged 3 commits into from
Jul 11, 2023
Merged

feat(outputs): allow to set multiple outputs (#648) #1346

merged 3 commits into from
Jul 11, 2023

Conversation

olivierboudet
Copy link
Contributor

@olivierboudet olivierboudet commented Jun 10, 2023 •
edited
Loading

I tried to resolve #648 with this PR but I am not a Go developer so you may have remarks...

This PR allows to specify multiple outputs like in syft, for example -o json=report.json -o template=report.html -t template.html

I need to add more tests but if someone could tell me if it looks ok overall ?

@olivierboudet olivierboudet force-pushed the feature/multiple_outputs branch 3 times, most recently from 002bf75 to 93474b2 Compare June 15, 2023 20:57
kzantow
kzantow reviewed Jun 30, 2023
View reviewed changes
Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

Are you able to add tests for this change?

cmd/writer.go Outdated Show resolved Hide resolved
@olivierboudet olivierboudet force-pushed the feature/multiple_outputs branch 4 times, most recently from ec1360d to 21e25b9 Compare July 3, 2023 20:52
@olivierboudet
Copy link
Contributor Author

Are you able to add tests for this change?

I just added some tests

@olivierboudet olivierboudet force-pushed the feature/multiple_outputs branch from 21e25b9 to 1ac5a4e Compare July 3, 2023 21:05
@wagoodman wagoodman self-assigned this Jul 6, 2023
@wagoodman
Copy link
Contributor

wagoodman commented Jul 10, 2023 •
edited
Loading

Right now this doesn't seem to work when providing multiple files:

$ go run . alpine:3.2 -o json=/tmp/json -o table -o cyclonedx=/tmp/cyclone

# stdout is cyclonedx, not the table output

That being said, I think I can update this PR to include additional pattern changes to how we encode the final reports (which should make this change easier to make). I'll think about it this afternoon and try to get something pushed before the end of the day.

wagoodman
wagoodman reviewed Jul 10, 2023
View reviewed changes
test/quality/vulnerability-match-labels Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

was updating this submodule intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it was not intentional, sorry for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

all good 😄

wagoodman
wagoodman reviewed Jul 10, 2023
View reviewed changes
grype/presenter/json/presenter.go Outdated
}
}

// Present creates a JSON-based reporting
func (pres *Presenter) Present(output io.Writer) error {
func (pres *Presenter) Present(defaultOutput io.Writer) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

unfortunately this is breaking the presenter abstraction by adding filesystem concerns.

Today the presenter acts as a two-way facade... the caller provides doesn't need to know about any format concerns (is this json, table, xml, etc) and the presenter only needs to write bytes to the writer, but does not need to know about the concerns of that writer (is it to a file, the screen, streaming to a socket, etc).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think ultimately we should probably be getting rid of the presenter abstraction and inverting this entire flow... so instead of providing a writer to an object, we should ask the object to return bytes. If this is the way to go, we should try to do this incrementally. A good approach to this would be to take the GetPresenter() function and adapt that to return the new object and write a short generalized helper function that takes a presenter and returns bytes. Initially we'd leave all of the presenters alone and fix everything above the GetPresenter() call (maybe call it GetWriter instead since it would no longer return a presenter). Then over time we can swap all of the presenters to encoders/writers.

I haven't thought this entirely though, but these are some initial thoughts.

Copy link
Contributor

@wagoodman wagoodman Jul 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@olivierboudet I can try out the above suggestion on your branch, as it would ultimately enable the multi -o feature (which would be awesome). Would you mind if I pushed updates to your branch? Or would you like to take a stab at these suggestions? (or of course, shout out if you have other suggestions here too!)

(Alternatively I could try and get these changes in another PR then rebase this, which isn't a problem, but I feel the rebase would be nasty and the features on this branch nicely motivate the need for the change.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review ! Of course you can push on my branch ☺️

@wagoodman wagoodman force-pushed the feature/multiple_outputs branch from 1ac5a4e to 79cf8d0 Compare July 11, 2023 17:14
olivierboudet and others added 2 commits July 11, 2023 13:15
@olivierboudet @wagoodman
feat(outputs): allow to set multiple outputs (anchore#648)
7d02d25 
Signed-off-by: Olivier Boudet <[email protected]>
Signed-off-by: Olivier Boudet <[email protected]>
Signed-off-by: Alex Goodman <[email protected]>
@olivierboudet @wagoodman
feat(outputs): allow to set multiple outputs (anchore#648)
ba08acf 
review

Signed-off-by: Olivier Boudet <[email protected]>
Signed-off-by: Alex Goodman <[email protected]>
@wagoodman wagoodman force-pushed the feature/multiple_outputs branch from 79cf8d0 to 46c543c Compare July 11, 2023 17:16
@wagoodman
Copy link
Contributor

@olivierboudet I updated the PR based on #1346 (comment) by leveraging the multi-writer patterns in syft. In doing so I needed to change some underlying eventing and presentation functionality, which we were overdue for. This doesn't address larger concerns with the presenter pattern and how it works with the execution flow, but instead nudges grype in the right direction (larger refactors can be done in future PRs)

wagoodman
wagoodman approved these changes Jul 11, 2023
View reviewed changes
Copy link
Contributor

@wagoodman wagoodman left a comment

Choose a reason for hiding this comment

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

this one has been a long time coming!

@wagoodman wagoodman force-pushed the feature/multiple_outputs branch from 01f98d8 to 269f7f0 Compare July 11, 2023 17:26
@wagoodman wagoodman enabled auto-merge (squash) July 11, 2023 17:27
@wagoodman wagoodman merged commit 9050883 into anchore:main Jul 11, 2023
@olivierboudet
Copy link
Contributor Author

Thanks a lot @wagoodman

@p-linnane p-linnane mentioned this pull request Jul 13, 2023
Merged
@dependabot dependabot bot mentioned this pull request Jul 14, 2023
Closed
@abg abg mentioned this pull request Jul 14, 2023
Closed
This was referenced Jul 18, 2023
Closed
Closed
@dependabot dependabot bot mentioned this pull request Jul 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kzantow kzantow kzantow left review comments

@wagoodman wagoodman wagoodman approved these changes

Assignees

@wagoodman wagoodman

Labels
None yet
Projects
OSS
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Write to multiple output files (like syft)
3 participants
@olivierboudet @wagoodman @kzantow