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

fix: improve error message for plugin #870

Merged

Conversation

JeyJeyGao
Copy link
Contributor

@JeyJeyGao JeyJeyGao commented Jan 17, 2024

Resolves #867
Signed-off-by: Junjie Gao junjiegao@microsoft.com

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
@codecov-commenter
Copy link

codecov-commenter commented Jan 18, 2024

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (ee72394) 65.14% compared to head (b2e8418) 64.93%.

Files Patch % Lines
cmd/notation/plugin/install.go 25.00% 4 Missing and 2 partials ⚠️
cmd/notation/plugin/list.go 0.00% 5 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #870      +/-   ##
==========================================
- Coverage   65.14%   64.93%   -0.22%     
==========================================
  Files          45       45              
  Lines        2717     2729      +12     
==========================================
+ Hits         1770     1772       +2     
- Misses        787      795       +8     
- Partials      160      162       +2     

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

shizhMSFT
shizhMSFT previously approved these changes Jan 18, 2024
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

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

Blocking the merge as I'm building from this branch and testing the new error messages locally.

@Two-Hearts
Copy link
Contributor

Two-Hearts commented Jan 18, 2024

When running command on a malformed plugin executable file on Windows:
./notation.exe plugin install --file ./malformed-plugin/azure-kv/notation-azure-kv.exe, the command returns following error message:

Error: plugin installation failed: failed to get metadata of new plugin: failed to execute the get-plugin-metadata command for plugin azure-kv: fork/exec ./malformed-plugin/azure-kv/notation-azure-kv.exe: This version of %1 is not compatible with the version of Windows you're running. Check your computer's system information and then contact the software publisher..
Please ensure the plugin is executable and meet the installation requirements on the windows/amd64.

On linux:

Error: plugin installation failed: failed to get metadata of new plugin: failed to execute the get-plugin-metadata command for plugin azure-kv: fork/exec test-plugin/malformed-plugin/azure-kv/notation-azure-kv: exec format error.
Please ensure the plugin is executable and meet the installation requirements on the linux/amd64.

These error messages are quite verbose though, can we improve them? (contents starting with fork/exec are already in the DEBUG log)

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
@JeyJeyGao
Copy link
Contributor Author

When running command on a malformed plugin executable file on Windows: ./notation.exe plugin install --file ./malformed-plugin/azure-kv/notation-azure-kv.exe, the command returns following error message:

Error: plugin installation failed: failed to get metadata of new plugin: failed to execute the get-plugin-metadata command for plugin azure-kv: fork/exec ./malformed-plugin/azure-kv/notation-azure-kv.exe: This version of %1 is not compatible with the version of Windows you're running. Check your computer's system information and then contact the software publisher..
Please ensure the plugin is executable and meet the installation requirements on the windows/amd64.

On linux:

Error: plugin installation failed: failed to get metadata of new plugin: failed to execute the get-plugin-metadata command for plugin azure-kv: fork/exec test-plugin/malformed-plugin/azure-kv/notation-azure-kv: exec format error.
Please ensure the plugin is executable and meet the installation requirements on the linux/amd64.

These error messages are quite verbose though, can we improve them? (contents starting with fork/exec are already in the DEBUG log)

Updated to:

./bin/notation plugin install --file ~/download/notation-azure-kv_1.0.1_darwin_amd64.tar.gz -d
DEBU[2024-01-18T12:55:26+08:00] Extracting file notation-azure-kv...         
DEBU[2024-01-18T12:55:26+08:00] Extracting file LICENSE...                   
DEBU[2024-01-18T12:55:26+08:00] Installing plugin from plugin path /tmp/notation-plugin2182206927 
DEBU[2024-01-18T12:55:26+08:00] Plugin get-plugin-metadata request: {}       
ERRO[2024-01-18T12:55:26+08:00] plugin get-plugin-metadata execution status: fork/exec /tmp/notation-plugin2182206927/notation-azure-kv: exec format error 
Error: plugin installation failed: failed to get metadata of new plugin: failed to execute the get-plugin-metadata command for plugin azure-kv.
Please ensure the plugin is executable and meet the installation requirements on the linux/amd64.

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
Two-Hearts
Two-Hearts previously approved these changes Jan 18, 2024
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

go.mod Outdated Show resolved Hide resolved
@Two-Hearts Two-Hearts dismissed their stale review January 18, 2024 08:12

pending on dependency PR

Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
cmd/notation/plugin/install.go Outdated Show resolved Hide resolved
cmd/notation/plugin/install.go Outdated Show resolved Hide resolved
cmd/notation/plugin/list.go Outdated Show resolved Hide resolved
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
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

@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

@Two-Hearts Two-Hearts merged commit dc786bc into notaryproject:main Jan 22, 2024
5 checks passed
rgnote pushed a commit to rgnote/notation that referenced this pull request Mar 8, 2024
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
rgnote pushed a commit to rgnote/notation that referenced this pull request Mar 8, 2024
Signed-off-by: Junjie Gao <junjiegao@microsoft.com>
Signed-off-by: rgnote <5878554+rgnote@users.noreply.github.com>
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.

Improve notation plugin error message
6 participants