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

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

Merged

Conversation

JeyJeyGao
Copy link
Contributor

@JeyJeyGao JeyJeyGao commented Jan 16, 2025

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

Copy link

codecov bot commented Jan 16, 2025

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 <[email protected]>
Signed-off-by: Junjie Gao <[email protected]>
cmd/notation/internal/output/print.go Outdated Show resolved Hide resolved
cmd/notation/inspect.go Outdated Show resolved Hide resolved
cmd/notation/internal/display/metadata/interface.go Outdated Show resolved Hide resolved
cmd/notation/internal/display/metadata/interface.go Outdated Show resolved Hide resolved
cmd/notation/internal/display/metadata/interface.go Outdated Show resolved Hide resolved
cmd/notation/internal/display/metadata/model/signature.go Outdated Show resolved Hide resolved
Signed-off-by: Junjie Gao <[email protected]>
Signed-off-by: Junjie Gao <[email protected]>
Signed-off-by: Junjie Gao <[email protected]>
Signed-off-by: Junjie Gao <[email protected]>
Signed-off-by: Junjie Gao <[email protected]>
Signed-off-by: Junjie Gao <[email protected]>
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao JeyJeyGao marked this pull request as ready for review January 20, 2025 08:39
@JeyJeyGao JeyJeyGao changed the title refactor: add inspect display handler refactor: extract inspect rendering logic to be display handlers Jan 20, 2025
cmd/notation/internal/option/format.go Show resolved Hide resolved
cmd/notation/internal/option/format.go Outdated Show resolved Hide resolved
cmd/notation/internal/option/common.go Show resolved Hide resolved
cmd/notation/inspect.go Outdated Show resolved Hide resolved
cmd/notation/internal/display/metadata/interface.go Outdated Show resolved Hide resolved
cmd/notation/internal/display/metadata/tree/inspect.go Outdated Show resolved Hide resolved
cmd/notation/internal/display/metadata/tree/inspect.go Outdated Show resolved Hide resolved
cmd/notation/internal/display/metadata/tree/inspect.go Outdated Show resolved Hide resolved
cmd/notation/internal/display/metadata/tree/inspect.go Outdated Show resolved Hide resolved
cmd/notation/internal/display/metadata/tree/inspect.go Outdated Show resolved Hide resolved
Signed-off-by: Junjie Gao <[email protected]>
Signed-off-by: Junjie Gao <[email protected]>
@JeyJeyGao JeyJeyGao requested a review from shizhMSFT January 21, 2025 01:54
Signed-off-by: Junjie Gao <[email protected]>
shizhMSFT
shizhMSFT previously approved these changes Jan 22, 2025
Copy link
Contributor

@shizhMSFT shizhMSFT 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: Junjie Gao <[email protected]>
cmd/notation/internal/output/print.go Outdated Show resolved Hide resolved
internal/tree/tree.go Outdated Show resolved Hide resolved
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

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.

@JeyJeyGao
Copy link
Contributor Author

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.

shizhMSFT
shizhMSFT previously approved these changes Jan 23, 2025
Copy link
Contributor

@shizhMSFT shizhMSFT 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
shizhMSFT previously approved these changes Jan 23, 2025
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

cmd/notation/internal/display/handler.go Outdated Show resolved Hide resolved
cmd/notation/internal/display/metadata/tree/inspect.go Outdated Show resolved Hide resolved
cmd/notation/internal/option/common.go Show resolved Hide resolved
cmd/notation/internal/option/format.go Outdated Show resolved Hide resolved
cmd/notation/internal/option/format.go Outdated Show resolved Hide resolved
cmd/notation/inspect.go Show resolved Hide resolved
Signed-off-by: Junjie Gao <[email protected]>
Two-Hearts
Two-Hearts previously approved these changes Feb 6, 2025
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

Signed-off-by: Junjie Gao <[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
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@JeyJeyGao JeyJeyGao merged commit 80cb6ee into notaryproject:main Feb 7, 2025
7 checks passed
@JeyJeyGao JeyJeyGao deleted the refactor/inpsect_output_handler branch February 7, 2025 23:16
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.

3 participants