-
Notifications
You must be signed in to change notification settings - Fork 4
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
chore: adding more required functionality #5
Conversation
priteshbandi
commented
Dec 23, 2023
•
edited
Loading
edited
- Adding more functionality required for notation-go to depend on this pkg.
- Also some renaming and update to Error struct to matching it with notation-go.
Signed-off-by: Pritesh Bandi <[email protected]>
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.
- Please add doc to struct
CLI
defined at line 23 ofcli.go
. - When I new a
CLI struct
withpl == nil
:cli, _ := New("notation-my-plugin", nil)
, this willpanic
when I callcli.Execute(...)
. We should always checknil
when new aCLI
. - In
NewWithLogger
function, we are only checking prefix of the plugin executable name. We should also check the suffix matches with the plugin'smetadata.Name
, i.e.{plugin-name}
ofnotation-{plugin-name}
should equal topl.GetMetadata resp.Name
. In fact, do we really need theexecutableName
parameter here? deferStdout()
andtype discardLogger struct{}
should be moved toutils.go
- In
utils.go getValidArgs()
, the result strings should not be hardcoded, because they are defined in theproto.go
:type Command string
Co-authored-by: Patrick Zheng <[email protected]> Signed-off-by: Pritesh Bandi <[email protected]>
Co-authored-by: Patrick Zheng <[email protected]> Signed-off-by: Pritesh Bandi <[email protected]>
ff76a38
to
591e3f3
Compare
Signed-off-by: Pritesh Bandi <[email protected]>
Done. Addressed all the feedbacks |
Signed-off-by: Pritesh Bandi <[email protected]>
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 with a single nit comment.
Signed-off-by: Pritesh Bandi <[email protected]>
Signed-off-by: Pritesh Bandi <[email protected]>
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
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
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