Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: handle stderr and stdout messages from plugins #1258
feat: handle stderr and stdout messages from plugins #1258
Changes from 8 commits
898f0dc
e4cf215
6f4c58b
e8d08fe
b43a4bc
3da7622
734c7af
2298323
56295fd
3757608
3c47c0d
bb8a4f4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 116 in pkg/common/plugin/exec.go
Codecov / codecov/patch
pkg/common/plugin/exec.go#L108-L116
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.
just curious if this could handle log like:
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.
Good question. The short answer is no. However, I did write code that can handle it. I'm sure it could be improved, but here's what I had come up and then decided against including it to keep it simpler.
Check warning on line 173 in pkg/common/plugin/exec.go
Codecov / codecov/patch
pkg/common/plugin/exec.go#L173
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.
ratify could swith formatter between text, json and logStash: https://github.com/deislabs/ratify/blob/main/internal/logger/logger.go#L135
just curious if this logger would switch accordingly.
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.
Hey @binbin-li, thanks for the comment. I think that would be a good thing to add to match the logging for the rest of the project.
@susanshi @akashsinghal, what do you both think? Worth adding in for this PR?
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.
I suggest to log an issue to track this improvement for the plugin log so we could merge this one first? what do you think @binbin-li
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.
Logged an issue to keep track of this feature request. :) #1304
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.
wondering when should we use std out vs err
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.
From what I've seen stderr is the default for messages, but most logging packages provide the option for redirecting to stdout. Tbh, I don't know which is best but wanted to give the flexibility to use both like logrus does. If the team has a strong opinion, I can add comments to nudge people to use stderr, which is the default for the pluginlogger I added.
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.
I see, thanks for the clarification. :) if we could just add these comment to the sample would be good. Exactly what you have mentioned. " stderr is the default for messages, but we do provide the option to redirecting to stdout"
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.
Comments added to the latest commit. :) Thanks for the feedback!