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

chore: adding more required functionality #5

Merged
merged 7 commits into from
Jan 3, 2024

Conversation

priteshbandi
Copy link
Contributor

@priteshbandi priteshbandi commented Dec 23, 2023

  • 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.

Verified

This commit was signed with the committer’s verified signature.
priteshbandi Pritesh Bandi
Signed-off-by: Pritesh Bandi <[email protected]>
Copy link
Contributor

@Two-Hearts Two-Hearts left a comment

Choose a reason for hiding this comment

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

  1. Please add doc to struct CLI defined at line 23 of cli.go.
  2. When I new a CLI struct with pl == nil: cli, _ := New("notation-my-plugin", nil), this will panic when I call cli.Execute(...). We should always check nil when new a CLI.
  3. In NewWithLogger function, we are only checking prefix of the plugin executable name. We should also check the suffix matches with the plugin's metadata.Name, i.e. {plugin-name} of notation-{plugin-name} should equal to pl.GetMetadata resp.Name. In fact, do we really need the executableName parameter here?
  4. deferStdout() and type discardLogger struct{} should be moved to utils.go
  5. In utils.go getValidArgs(), the result strings should not be hardcoded, because they are defined in the proto.go:
    type Command string

example/main.go Outdated Show resolved Hide resolved
example/main.go Outdated Show resolved Hide resolved
plugin/proto.go Show resolved Hide resolved
priteshbandi and others added 2 commits December 25, 2023 21:16

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Patrick Zheng <[email protected]>
Signed-off-by: Pritesh Bandi <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Patrick Zheng <[email protected]>
Signed-off-by: Pritesh Bandi <[email protected]>
@priteshbandi priteshbandi force-pushed the main branch 2 times, most recently from ff76a38 to 591e3f3 Compare December 30, 2023 09:10

Verified

This commit was signed with the committer’s verified signature.
priteshbandi Pritesh Bandi
Signed-off-by: Pritesh Bandi <[email protected]>
@priteshbandi
Copy link
Contributor Author

  1. Please add doc to struct CLI defined at line 23 of cli.go.

    1. When I new a CLI struct with pl == nil: cli, _ := New("notation-my-plugin", nil), this will panic when I call cli.Execute(...). We should always check nil when new a CLI.

    2. In NewWithLogger function, we are only checking prefix of the plugin executable name. We should also check the suffix matches with the plugin's metadata.Name, i.e. {plugin-name} of notation-{plugin-name} should equal to pl.GetMetadata resp.Name. In fact, do we really need the executableName parameter here?

    3. deferStdout() and type discardLogger struct{} should be moved to utils.go

    4. In utils.go getValidArgs(), the result strings should not be hardcoded, because they are defined in the proto.go:

      type Command string

Done. Addressed all the feedbacks

feedback

Verified

This commit was signed with the committer’s verified signature.
priteshbandi Pritesh Bandi
Signed-off-by: Pritesh Bandi <[email protected]>
Copy link
Contributor

@Two-Hearts Two-Hearts left a 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.

cli/cli.go Show resolved Hide resolved
cli/utils.go Outdated Show resolved Hide resolved
cli/cli.go Outdated Show resolved Hide resolved

Verified

This commit was signed with the committer’s verified signature.
priteshbandi Pritesh Bandi
Signed-off-by: Pritesh Bandi <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
priteshbandi Pritesh Bandi
Signed-off-by: Pritesh Bandi <[email protected]>
Copy link
Contributor

@Two-Hearts Two-Hearts left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@rgnote rgnote left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@JeyJeyGao JeyJeyGao left a comment

Choose a reason for hiding this comment

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

LGTM

@priteshbandi priteshbandi merged commit c077eda into notaryproject:main Jan 3, 2024
1 check passed
@priteshbandi priteshbandi mentioned this pull request Jan 29, 2024
6 tasks
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.

None yet

5 participants