-
Notifications
You must be signed in to change notification settings - Fork 64
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
Changes from all commits
898f0dc
e4cf215
6f4c58b
e8d08fe
b43a4bc
3da7622
734c7af
2298323
56295fd
3757608
3c47c0d
bb8a4f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
/* | ||
Copyright The Ratify Authors. | ||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package logger | ||
|
||
import ( | ||
"fmt" | ||
"io" | ||
"log" | ||
"os" | ||
) | ||
|
||
type Logger struct { | ||
infoLogger *log.Logger | ||
debugLogger *log.Logger | ||
warnLogger *log.Logger | ||
} | ||
|
||
func NewLogger() *Logger { | ||
return &Logger{ | ||
infoLogger: log.New(os.Stderr, "INFO: ", 0), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Logged an issue to keep track of this feature request. :) #1304 |
||
debugLogger: log.New(os.Stderr, "DEBUG: ", 0), | ||
warnLogger: log.New(os.Stderr, "WARN: ", 0), | ||
} | ||
} | ||
|
||
func (l *Logger) SetOutput(out io.Writer) { | ||
l.infoLogger.SetOutput(out) | ||
l.debugLogger.SetOutput(out) | ||
l.warnLogger.SetOutput(out) | ||
} | ||
|
||
func (l *Logger) Info(message string) { | ||
l.infoLogger.Println(message) | ||
} | ||
|
||
func (l *Logger) Infof(format string, args ...interface{}) { | ||
message := fmt.Sprintf(format, args...) | ||
l.infoLogger.Println(message) | ||
} | ||
|
||
func (l *Logger) Debug(message string) { | ||
l.debugLogger.Println(message) | ||
} | ||
|
||
func (l *Logger) Debugf(format string, args ...interface{}) { | ||
message := fmt.Sprintf(format, args...) | ||
l.debugLogger.Println(message) | ||
} | ||
|
||
func (l *Logger) Warn(message string) { | ||
l.warnLogger.Println(message) | ||
} | ||
|
||
func (l *Logger) Warnf(format string, args ...interface{}) { | ||
message := fmt.Sprintf(format, args...) | ||
l.warnLogger.Println(message) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,8 +18,10 @@ package main | |
import ( | ||
"encoding/json" | ||
"fmt" | ||
"os" | ||
|
||
"github.com/deislabs/ratify/pkg/common" | ||
"github.com/deislabs/ratify/pkg/common/plugin/logger" | ||
"github.com/deislabs/ratify/pkg/ocispecs" | ||
"github.com/deislabs/ratify/pkg/referrerstore" | ||
"github.com/deislabs/ratify/pkg/verifier" | ||
|
@@ -37,7 +39,19 @@ type PluginInputConfig struct { | |
} | ||
|
||
func main() { | ||
// create a plugin logger | ||
pluginlogger := logger.NewLogger() | ||
|
||
// output info and Debug to stderr | ||
pluginlogger.Info("initialized plugin") | ||
pluginlogger.Debugf("plugin %s %s", "sample", "1.0.0") | ||
|
||
skel.PluginMain("sample", "1.0.0", VerifyReference, []string{"1.0.0"}) | ||
|
||
// By default, the pluginlogger writes to stderr. To change the output, use SetOutput | ||
pluginlogger.SetOutput(os.Stdout) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Comments added to the latest commit. :) Thanks for the feedback! |
||
// output warning to stdout | ||
pluginlogger.Warn("example warning message...") | ||
susanshi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
func parseInput(stdin []byte) (*PluginConfig, error) { | ||
|
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.