Skip to content

Comments

refactor: refactor internal packages#1205

Merged
9 commits merged intomainfrom
unknown repository
Mar 10, 2025
Merged

refactor: refactor internal packages#1205
9 commits merged intomainfrom
unknown repository

Conversation

@ghost
Copy link

@ghost ghost commented Mar 7, 2025

As discussed with @JeyJeyGao, this PR completes the refactoring work for Notation internal packages.

The idea is, any package directly related to command shall be put under cmd/notation/internal dir, whereas packages related to functioning and logic shall be put under internal.

Resolves #1151

Patrick Zheng added 2 commits March 7, 2025 12:08
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
@codecov
Copy link

codecov bot commented Mar 7, 2025

Codecov Report

Attention: Patch coverage is 85.87571% with 25 lines in your changes missing coverage. Please review.

Project coverage is 76.13%. Comparing base (fb9dbf7) to head (8bb1fbb).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
cmd/notation/internal/verify/verify.go 76.92% 7 Missing and 2 partials ⚠️
cmd/notation/cert/list.go 84.21% 2 Missing and 1 partial ⚠️
cmd/notation/internal/flag/options.go 81.25% 2 Missing and 1 partial ⚠️
cmd/notation/internal/sign/sign.go 72.72% 2 Missing and 1 partial ⚠️
cmd/notation/key.go 85.71% 2 Missing and 1 partial ⚠️
cmd/notation/blob/sign.go 85.71% 1 Missing ⚠️
cmd/notation/internal/display/handler.go 90.00% 1 Missing ⚠️
cmd/notation/list.go 75.00% 1 Missing ⚠️
cmd/notation/registry.go 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1205      +/-   ##
==========================================
- Coverage   76.24%   76.13%   -0.12%     
==========================================
  Files          70       66       -4     
  Lines        3637     3628       -9     
==========================================
- Hits         2773     2762      -11     
- Misses        668      669       +1     
- Partials      196      197       +1     

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Patrick Zheng added 2 commits March 7, 2025 13:07
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
@ghost ghost changed the title refactor: refactor Notation internal packages refactor: refactor internal packages Mar 7, 2025
Patrick Zheng added 2 commits March 7, 2025 15:37
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
@ghost ghost mentioned this pull request Mar 7, 2025
JeyJeyGao
JeyJeyGao previously approved these changes Mar 7, 2025
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

@shizhMSFT shizhMSFT requested a review from Copilot March 7, 2025 08:27
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR refactors the internal packages by reorganizing command‐related packages under cmd/notation/internal and moving functional logic into internal. Key changes include updating package names and import paths, consolidating duplicate functionalities, and aligning flag options and display handling with the new package structure.

Reviewed Changes

File Description
cmd/notation/internal/sign/sign_test.go Updated package name and import paths to reflect refactoring.
cmd/notation/internal/flag/options.go Renamed package from cmd to flag and updated configuration usage.
cmd/notation/cert/list.go Replaced deprecated ioutil.PrintCertMap with local printCertMap.
cmd/notation/internal/sign/sign.go Updated import paths for flag options and removed unused deps.
cmd/notation/internal/display/display.go Updated package comment and documentation for display handling.
(Other files) Consistent refactoring of flag, signer, display, and command logic.

Copilot reviewed 52 out of 52 changed files in this pull request and generated 1 comment.

Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
@ghost ghost requested a review from JeyJeyGao March 7, 2025 09:03
@shizhMSFT shizhMSFT requested a review from Copilot March 7, 2025 10:37
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR refactors the internal packages to improve the separation between command‐related and logic/utility packages, as discussed with @JeyJeyGao.

  • Relocates packages directly related to commands under cmd/notation/internal (e.g. sign, flag, display) while keeping logic and configuration packages under internal or new subdirectories.
  • Updates import paths, flag handling, and test references accordingly, and removes legacy code such as the old output format options.

Reviewed Changes

File Description
cmd/notation/internal/sign/sign_test.go Renames package from “signer” to “sign” and updates flag usage to reflect the new path.
cmd/notation/cert/list.go Replaces references to internal cmd with the new flag package and updates certificate printing logic.
cmd/notation/internal/flag/options.go Moves the file from the “cmd” package to the “flag” package and refactors output flag handling.
cmd/notation/internal/sign/sign.go Updates import paths to use the flag package and refactors key resolution logic accordingly.
cmd/notation/internal/display/display.go Revises package documentation/comments and updates the confirmation function implementation.
cmd/notation/internal/display/output/format.go Renames the package from “option” to “output” and defines output format constants.
cmd/notation/internal/display/output/print.go Adjusts comments and renames for improved clarity.
cmd/notation/blob/verify.go Updates import paths and flag usage in preparation for refactored key verification.
cmd/notation/inspect.go Updates flag usage and output format handling for the inspect command.
cmd/notation/blob/inspect.go Replaces usage of legacy option flags with the new flag package for inspection.
cmd/notation/internal/flag/flags.go Adjusts configuration loading to make use of the new config package.
cmd/notation/internal/flag/options_test.go Renames the package to “flag” to reflect the updated internal structure.
cmd/notation/inspect_test.go Updates test expectations to match new error messages and flag parsing behavior.
cmd/notation/internal/display/handler.go Updates the NewInspectHandler and NewBlobInspectHandler functions to use new output formats.
cmd/notation/blob/sign_test.go Refactors test cases to use the updated flag package instead of the legacy cmd package.
cmd/notation/blob/sign.go Replaces internal references (e.g. signer, confirmation routines) with updated flag and display packages and updates error messages accordingly.
cmd/notation/cert/show.go Switches from the internal cmd package to the updated flag package for logging options.
cmd/notation/blob/policy/import.go Updates the confirmation prompt to use the new display package for consistency.
cmd/notation/internal/option/format.go Removes legacy code that has been superseded by the new flag/output refactoring.

Copilot reviewed 52 out of 52 changed files in this pull request and generated no comments.

Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
@ghost ghost requested a review from shizhMSFT March 10, 2025 05:58
shizhMSFT
shizhMSFT previously approved these changes Mar 10, 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: Patrick Zheng <patrickzheng@microsoft.com>
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

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

@ghost ghost merged commit d340541 into notaryproject:main Mar 10, 2025
7 checks passed
@ghost ghost deleted the refactoring branch March 10, 2025 06:28
7h3-3mp7y-m4n pushed a commit to 7h3-3mp7y-m4n/notation that referenced this pull request Mar 29, 2025
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
FeynmanZhou pushed a commit to FeynmanZhou/notation that referenced this pull request May 15, 2025
Signed-off-by: Patrick Zheng <patrickzheng@microsoft.com>
This pull request was closed.
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.

housekeeping: refactoring for sustainable coding

2 participants