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.

Sign up for GitHub

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

Include connectivity to server check in invalid server or credentials error #2545

Merged
merged 1 commit into from
Jan 27, 2025
+1 −1
Merged

Include connectivity to server check in invalid server or credentials error #2545

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/clients/connect/client_connect.go
View file
Original file line number Diff line number Diff line change
@@ -86,7 +86,7 @@ func (u *UserDTO) CanPublish() bool {
return u.UserRole == AuthRoleAdmin || u.UserRole == AuthRolePublisher
}

var errInvalidServerOrCredentials = errors.New("could not validate credentials; check server URL and API key or token")
var errInvalidServerOrCredentials = errors.New("could not validate credentials; check connectivity to the server, the URL, and API key")
var errInvalidApiKey = errors.New("could not log in with the provided credentials")
var errInvalidServer = errors.New("could not access the server; check the server URL and try again")
Copy link
Collaborator

Choose a reason for hiding this comment

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

With a Server URL connectivity issue when creating credentials, I encountered this error. Did we not want to also update this message to check connectivity?

2025-01-22 at 8 31 AM

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we also update the second and third errors to show the server URL with which we're trying to connect?

2025-01-22 at 8 33 AM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did we not want to also update this message to check connectivity?

For this error it does include "could not access the server" which to me covers the case where the server is down. Were you thinking something different with the error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we also update the second and third errors to show the server URL with which we're trying to connect

Since those two are used during or right after inputing the URL I don't think it is necessary to repeat it like that. I could be more convinced on the second, but then I worry about error length.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For this error it does include "could not access the server" which to me covers the case where the server is down. Were you thinking something different with the error?

Fair enough.

Since those two are used during or right after inputting the URL, I don't think it is necessary to repeat it like that. I could be more convinced on the second, but then I worry about error length.

You are correct in that connectivity to the URL has been verified when we get to this step. So when we fail here, the API key may no longer be valid or our validation of the server URL has issues. I think it would be useful to have the server URL present if I were trying to track it down (I think instant visual validation might help), and I'm more interested in providing the information to the user rather than the brevity of the error message. But I'm not strongly tied to this, I'll leave it to you to weigh the benefits and costs.