Skip to content

Add spec for verify json output#559

Closed
byronchien wants to merge 1 commit intonotaryproject:mainfrom
byronchien:json-verify-spec
Closed

Add spec for verify json output#559
byronchien wants to merge 1 commit intonotaryproject:mainfrom
byronchien:json-verify-spec

Conversation

@byronchien
Copy link
Contributor

Adds spec for notation verify $IMAGE --output json.

Changes from previous iteration:

  • adds a details field for additional information related to the result
  • outputs a JSON object on failure and skipped results

Related issues and PRs:

@FeynmanZhou
Copy link
Member

@byronchien You may need to rebase your branch and sync with the upstream main as it has conflicts.

@codecov-commenter
Copy link

Codecov Report

Merging #559 (f71dc76) into main (0391202) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##             main     #559   +/-   ##
=======================================
  Coverage   63.52%   63.52%           
=======================================
  Files          40       40           
  Lines        2237     2237           
=======================================
  Hits         1421     1421           
  Misses        695      695           
  Partials      121      121           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

priteshbandi
priteshbandi previously approved these changes Jun 14, 2023
Copy link
Contributor

@priteshbandi priteshbandi left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Byron Chien <byronc@ucla.edu>
Copy link
Contributor

@priteshbandi priteshbandi 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 self-requested a review July 5, 2023 17:07
Copy link
Contributor

@priteshbandi priteshbandi left a comment

Choose a reason for hiding this comment

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

LGTM

@sajayantony
Copy link
Contributor

+1. I think it's worth merging this. Typically it would be good to call out all possible values of result if there are more.

Copy link
Member

@FeynmanZhou FeynmanZhou left a comment

Choose a reason for hiding this comment

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

LGTM

@shizhMSFT
Copy link
Contributor

Should it be merged post v1?

@yizha1
Copy link
Contributor

yizha1 commented Jul 8, 2023

Should it be merged post v1?

+1 on merging it post v1.


### Verify signatures on an OCI artifact with json output

Use the `--output` flag to format verification output in json.
Copy link

Choose a reason for hiding this comment

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

Can you also add this new flag along with its description into the Outline session of this spec? (https://github.com/notaryproject/notation/blob/main/specs/commandline/verify.md#outline)

Use the `--output` flag to format verification output in json.

```shell
notation verify --output json localhost:5000/net-monitor@sha256:b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9
Copy link

Choose a reason for hiding this comment

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

Just curious, what error message are we expecting from the following command:
notation verify --output <something_not_defined> localhost:5000/net-monitor@sha256:b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9

Copy link
Contributor

@yizha1 yizha1 left a comment

Choose a reason for hiding this comment

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

@byronchien I reviewed this PR again. Here are two comments:

  • Unified solution for all Notation CLI commands: I couldn't recall any reason behind this PR that only verify command is addressed. The Json output should be considered for all other Notation CLI commands. So, it is suggested to create a unified solution for all Notation CLI commands.
  • About the flag name -o --output: There is a new feature about signing and verifying arbitrary files. For this feature, we may need to output signature to a file during signing, there may be a conflict of using --output. It seems notation inspect command already uses the flag --output. We need to align the usage of flag --output.
    /cc: @priteshbandi @shizhMSFT @FeynmanZhou

@github-actions
Copy link

This PR is stale because it has been opened for 45 days with no activity. Remove stale label or comment. Otherwise, it will be closed in 30 days.

@github-actions github-actions bot added the Stale label Mar 23, 2024
@github-actions
Copy link

PR closed due to no activity in the past 30 days.

@github-actions github-actions bot closed this Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

Comments