-
Notifications
You must be signed in to change notification settings - Fork 38
fix: change error message if no organization exists #2105
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: change error message if no organization exists #2105
Conversation
Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
msg = "authentication required, please run \"chainloop auth login\"" | ||
case v1.IsUserNotMemberOfOrgErrorNotInOrg(err), v1.IsUserWithNoMembershipErrorNotInOrg(err): | ||
msg = "the organization you are trying to access does not exist, please run \"chainloop auth login\"" | ||
msg = "you are not part of any organization, please run \"chainloop organization create --name ORG_NAME\" to create 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.
thanks for the PR!
I think we might need to split the switch into the two cases, v1.IsUserNotMemberOfOrgErrorNotInOrg(err),
might not mean that tit doesn't have any org but instead that it's not part of this specific org.
Let me review the current custom error throwing, thanks!
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.
Hi, thanks again for providing a PR but I although I think it fixes the issue, it adds a regression where we will tell users that they do not have an organization when they just don't have access to it.
The following change, indeed fixes the issue
diff --git a/app/cli/main.go b/app/cli/main.go
index ff807fce..ca1b9540 100644
--- a/app/cli/main.go
+++ b/app/cli/main.go
@@ -89,8 +89,10 @@ func errorInfo(err error, logger zerolog.Logger) (string, int) {
msg = "your authentication token has expired, please run chainloop auth login again"
case isWrappedErr(st, jwtMiddleware.ErrMissingJwtToken):
msg = "authentication required, please run \"chainloop auth login\""
- case v1.IsUserNotMemberOfOrgErrorNotInOrg(err), v1.IsUserWithNoMembershipErrorNotInOrg(err):
- msg = "the organization you are trying to access does not exist, please run \"chainloop auth login\""
+ case v1.IsUserNotMemberOfOrgErrorNotInOrg(err):
+ msg = "the organization you are trying to access does not exist or you are not part of it, please run \"chainloop auth login\""
+ case v1.IsUserWithNoMembershipErrorNotInOrg(err):
+ msg = "you are not part of any organization, please run \"chainloop organization create --name ORG_NAME\" to create one"
case errors.As(err, &cmd.GracefulError{}):
// Graceful recovery if the flag is set and the received error is marked as recoverable
if cmd.GracefulExit {
what I do is to make sure the error handling is split between the two cases
a) when the user doesn't have any organization
b) when the user doesn't have access to the provided organization
The original issue #2088 was about making sure that during the attestation process, we show a) not b), and this change fixes it.
Feel free to apply those changes in your branch and let me know.
Thanks!
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.
Thanks for the feedback! You're right, that was causing a misleading error. I've applied your suggested changes to handle both cases separately.
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.
See my inline comment, thanks!
Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
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.
Thanks!
Congratulations for your first contribution! |
Fix for #2088