refactor: extract inspect rendering logic to be display handlers #1150

JeyJeyGao · 2025-01-16T09:21:40Z

Depends on spec change: #1156

Refactor:

Extract output formatting logic from the inspect command layer to an isolated display handler layer for processing rendering.
Add json and tree inspect handlers.

Fix:

For tree output, make the key names with multiple words separated by space characters rather than capitalizing the words, which is defined in the inspect command spec.
For json output, default to rendering time in RFC3339 with nanoseconds (Notation expiry, signing time and certificate expiry are accurate to 1 second. Timestamp RFC 3161 can have fraction-of-second time value).

E2E Test:

inspect signature with timestamp, signature expiry and user metadata (text, json)
inspect signatures with invalid timestamp (test, json)
inspect with -o shorthand.

Resolves part of #1151

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

codecov · 2025-01-16T09:24:14Z

Codecov Report

Attention: Patch coverage is 88.85135% with 33 lines in your changes missing coverage. Please review.

Project coverage is 74.70%. Comparing base (2920cae) to head (60035a7).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
...notation/internal/display/metadata/tree/inspect.go	87.12%	9 Missing and 4 partials ⚠️
...notation/internal/display/metadata/json/inspect.go	89.32%	8 Missing and 3 partials ⚠️
...md/notation/internal/display/metadata/tree/tree.go	60.00%	4 Missing and 2 partials ⚠️
cmd/notation/inspect.go	85.71%	0 Missing and 2 partials ⚠️
internal/envelope/envelope.go	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1150      +/-   ##
==========================================
+ Coverage   73.30%   74.70%   +1.40%     
==========================================
  Files          53       59       +6     
  Lines        3240     3337      +97     
==========================================
+ Hits         2375     2493     +118     
+ Misses        671      651      -20     
+ Partials      194      193       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

cmd/notation/internal/output/print.go

cmd/notation/inspect.go

cmd/notation/internal/display/metadata/interface.go

cmd/notation/internal/display/metadata/model/signature.go

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

cmd/notation/internal/option/format.go

cmd/notation/internal/option/common.go

cmd/notation/inspect.go

cmd/notation/internal/display/metadata/interface.go

cmd/notation/internal/display/metadata/tree/inspect.go

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

cmd/notation/internal/output/print.go

internal/tree/tree.go

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

shizhMSFT

I noticed that handlers are grouped by command names instead of format types. Although both are acceptable, grouping by format types can have more benefits in the long run, especially we have growing commands but fixed set of format types.

One important point is that grouping by format types make developers easier to have consistent output for a certain format type across all commands.

Also, it is easier to have common utility functions without creating a utility package, which is discouraged.

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

JeyJeyGao · 2025-01-23T08:26:20Z

I noticed that handlers are grouped by command names instead of format types. Although both are acceptable, grouping by format types can have more benefits in the long run, especially we have growing commands but fixed set of format types.

One important point is that grouping by format types make developers easier to have consistent output for a certain format type across all commands.

Also, it is easier to have common utility functions without creating a utility package, which is discouraged.

Updated to group the handlers by format types.

cmd/notation/internal/display/handler.go

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

shizhMSFT

LGTM

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

shizhMSFT

LGTM

cmd/notation/internal/display/handler.go

cmd/notation/internal/display/metadata/json/inspect.go

cmd/notation/internal/display/metadata/tree/inspect.go

cmd/notation/internal/display/metadata/tree/tree.go

cmd/notation/internal/option/common.go

cmd/notation/internal/option/format.go

cmd/notation/inspect.go

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

ghost

LGTM

cmd/notation/internal/display/metadata/tree/tree.go

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

ghost

LGTM

shizhMSFT

LGTM

…aryproject#1150) Depends on spec change: notaryproject#1156 Refactor: - Extract output formatting logic from the inspect command layer to an isolated display handler layer for processing rendering. - Add `json` and `tree` inspect handlers. Fix: - For `tree` output, make the key names with multiple words separated by space characters rather than capitalizing the words, which is defined in the [inspect command spec](https://github.com/notaryproject/notation/blob/v1.2.0/specs/commandline/inspect.md#inspect-signatures-on-the-supplied-oci-artifact-identified-by-the-digest). - For `json` output, default to rendering time in RFC3339 with nanoseconds (Notation expiry, signing time and certificate expiry are accurate to 1 second. Timestamp [RFC 3161](https://www.rfc-editor.org/rfc/rfc3161#section-2.4.2) can have fraction-of-second time value). E2E Test: - inspect signature with timestamp, signature expiry and user metadata (text, json) - inspect signatures with invalid timestamp (test, json) - inspect with `-o` shorthand. Resolves part of notaryproject#1151 --------- Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

refactor: add inpect display handler

89adf15

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

JeyJeyGao added 3 commits January 16, 2025 10:21

fix: update code

f0756a8

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

test: add unit test

578dea2

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

test: add e2e test for tree and json

24dea8b

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

shizhMSFT reviewed Jan 17, 2025

View reviewed changes

JeyJeyGao added 8 commits January 17, 2025 09:08

refactor code

4031faf

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

fix: update E2E

37b967a

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

fix: resolve comments

c2697db

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

fix: update license header

edbfbad

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

fix: update test

eb078e3

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

test: add unit test

4d3aedd

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

test: add unit test for inspect

8fdc989

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

test: update test

0640165

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

JeyJeyGao marked this pull request as ready for review January 20, 2025 08:39

JeyJeyGao requested review from NiazFK, gokarnm, priteshbandi, rgnote and toddysm as code owners January 20, 2025 08:39

JeyJeyGao requested a review from a user January 20, 2025 08:39

JeyJeyGao requested review from vaninrao10 and yizha1 as code owners January 20, 2025 08:39

JeyJeyGao changed the title ~~refactor: add inspect display handler~~ refactor: extract inspect rendering logic to be display handlers Jan 20, 2025

shizhMSFT requested changes Jan 20, 2025

View reviewed changes

JeyJeyGao added 2 commits January 21, 2025 01:44

fix: resolve comemnts

a1e411f

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

fix: test

84a1991

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

JeyJeyGao requested a review from shizhMSFT January 21, 2025 01:54

fix: remove envelope media type

8083c25

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

ghost mentioned this pull request Jan 21, 2025

docs: fix inspect command spec #1156

Merged

ghost reviewed Jan 23, 2025

View reviewed changes

cmd/notation/internal/output/print.go Show resolved Hide resolved

internal/tree/tree.go Show resolved Hide resolved

JeyJeyGao added 2 commits January 23, 2025 05:16

refactor: display package structure

aef5735

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

fix: update to use *ocispec.Descriptor

5b28eda

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

shizhMSFT reviewed Jan 23, 2025

View reviewed changes

fix: restore package structure

8f5d88f

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

shizhMSFT reviewed Jan 23, 2025

View reviewed changes

cmd/notation/internal/display/handler.go Outdated Show resolved Hide resolved

fix: restore "NewInpsectHandler" function name

1adc392

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

shizhMSFT previously approved these changes Jan 23, 2025

View reviewed changes

fix: move internal/tree to tree handlers package

da16190

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

JeyJeyGao dismissed shizhMSFT’s stale review via da16190 January 23, 2025 08:41

shizhMSFT previously approved these changes Jan 23, 2025

View reviewed changes

ghost suggested changes Feb 6, 2025

View reviewed changes

fix: resolve comments

7cf59d4

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

JeyJeyGao dismissed shizhMSFT’s stale review via 7cf59d4 February 6, 2025 03:46

ghost previously approved these changes Feb 6, 2025

View reviewed changes

shizhMSFT reviewed Feb 6, 2025

View reviewed changes

cmd/notation/internal/display/metadata/tree/tree.go Outdated Show resolved Hide resolved

fix: udpate new to newNode

60035a7

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>

JeyJeyGao dismissed ghost ’s stale review via 60035a7 February 7, 2025 01:32

JeyJeyGao requested review from a user and shizhMSFT February 7, 2025 05:10

ghost approved these changes Feb 7, 2025

View reviewed changes

shizhMSFT approved these changes Feb 7, 2025

View reviewed changes

JeyJeyGao merged commit 80cb6ee into notaryproject:main Feb 7, 2025
7 checks passed

JeyJeyGao deleted the refactor/inpsect_output_handler branch February 7, 2025 23:16

JeyJeyGao mentioned this pull request Feb 8, 2025

housekeeping: refactoring for sustainable coding #1151

Closed

5 tasks

This was referenced Mar 12, 2025

bump: release v2.0.0-alpha.1 #1215

Closed

bump: release v2.0.0-alpha.1 #1218

Merged

refactor: extract inspect rendering logic to be display handlers #1150

refactor: extract inspect rendering logic to be display handlers #1150

Uh oh!

Conversation

JeyJeyGao commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shizhMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

JeyJeyGao commented Jan 23, 2025

Uh oh!

Uh oh!

shizhMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

shizhMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

shizhMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JeyJeyGao commented Jan 16, 2025 •

edited

Loading

codecov bot commented Jan 16, 2025 •

edited

Loading