Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
doc: proposal for error message improvements #1662
doc: proposal for error message improvements #1662
Changes from 5 commits
2000c11
9dc9e82
70ba627
1d12f7f
4ae4332
a494009
1401080
4bbd9f1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example is also confusing. Why are they two HTTP requests (HEAD and GET) and how is this helping the user understand what the reason is (if this is meant to explain the reason)?
Also, having everything lowercase makes those messages really hard to read because it is not clear where a sentence ends and starts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@toddysm This message (HEAD and GET) was returned by ACR in this example. The ACR responded with an error and the response message was printed out. If we do not print out server response, we do not know what happened and the reason. In this case, the 401 unauthorized was also included in the message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing through errors is not a user-friendly practice. We need to catch the error from ACR and wrap it into something meaningful\ for the user of Rarify. The user of Ratify may have no knowledge of where the artifact is and even not know what artifact descriptor is. We need to provide meaningful message for the user of Ratyify and not the user of ACR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have one quick question. If we found the artifact was not signed, why the error reason is
no signature verification report is found
. It sounds more likeverification failed
ornot signed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's good call. I think this proposal is mainly focusing on the overall format of the error, like what info should be printed in the log. For the specific message for each error, we'll handle it one by one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fseldow Updated. Please review it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another question, how can customer get the remediation msg? via
kubectl logs ratify
orconstraint template violation msg
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fseldow as you said there are two ways, I prefer policy controller to customize the constraint template to use the
remediation
field, since it is not easy for users to identify the correct logs among many withkubectl logs
@binbin-li I think Ratify can also improve the current default constraint template to include the remediation message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fseldow Updated. Please review it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1669 I updated this issue to add tracking remediation in the CT.