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 #406

Merged
merged 7 commits into from
Jul 30, 2024

Conversation

JeyJeyGao
Copy link
Contributor

@JeyJeyGao JeyJeyGao commented May 28, 2024

Fix:

  • Simplified the error message for plugin errors.
  • Logged the details of the failing plugin command instead of printing them out in the CLI output, as users may not understand what the plugin command is, which can mislead them about the important part of the error message.
  • Logged the error code instead of printing it out in the CLI output to simplify the error message.

Previous example:

notation sign xxx.azurecr.io/hello-app:v1 --plugin azure-kv --id https://xxx.vault.azure.net/certificates --plugin-config credential_type=azurecli 
Warning: Always sign the artifact using digest(@sha256:...) rather than a tag(:v1) because tags are mutable and a tag reference can point to a different artifact than the one signed.
Error: failed to execute the describe-key command for azure-kv: VALIDATION_ERROR: Invalid input passed to "--id". Please follow this format to input the ID "https://<vault-name>.vault.azure.net/certificates/<certificate-name>/[certificate-version]"

Current example:

notation sign xxx.azurecr.io/hello-app:v1 --plugin azure-kv --id https://xxx.vault.azure.net/certificates --plugin-config credential_type=azurecli 
Warning: Always sign the artifact using digest(@sha256:...) rather than a tag(:v1) because tags are mutable and a tag reference can point to a different artifact than the one signed.
Error: failed to sign with the plugin azure-kv: Invalid input passed to "--id". Please follow this format to input the ID "https://<vault-name>.vault.azure.net/certificates/<certificate-name>/[certificate-version]"

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

JeyJeyGao added 4 commits May 30, 2024 09:49
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>
@JeyJeyGao JeyJeyGao force-pushed the fix/error_message_for_plugin branch from df0505e to ad729c4 Compare May 30, 2024 02:02
Copy link

codecov bot commented May 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.73%. Comparing base (b3b8cbe) to head (a94af32).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #406      +/-   ##
==========================================
- Coverage   80.75%   80.73%   -0.03%     
==========================================
  Files          33       33              
  Lines        2448     2455       +7     
==========================================
+ Hits         1977     1982       +5     
  Misses        331      331              
- Partials      140      142       +2     

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

@JeyJeyGao JeyJeyGao marked this pull request as ready for review May 30, 2024 02:10
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 Jul 15, 2024
plugin/plugin.go Outdated Show resolved Hide resolved
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.

PR needs rebase. Otherwise, LGTM.

@github-actions github-actions bot removed the Stale label Jul 16, 2024
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

@JeyJeyGao JeyJeyGao merged commit 09e32d7 into notaryproject:main Jul 30, 2024
9 checks passed
@Two-Hearts Two-Hearts mentioned this pull request Aug 16, 2024
6 tasks
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.

Plugin error messages not straightforward for users
3 participants