-
Notifications
You must be signed in to change notification settings - Fork 8
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
[CLD-6341] Add log link to Grafana for Matterwick comments #57
Conversation
@@ -196,7 +196,8 @@ func (s *Server) createCloudSpinWickWithCWS(pr *model.PullRequest, size string, | |||
} | |||
|
|||
userTable := fmt.Sprintf("| Account Type | Username | Password |\n|---|---|---|\n| Admin | %s | %s |", username, password) | |||
msg := fmt.Sprintf("Mattermost test server with CWS created! :tada:\n\nAccess here: %s\n\n%s", spinwickURL, userTable) | |||
logLink := fmt.Sprintf("https://grafana.internal.mattermost.com/explore?orgId=1&left=%%7B%%22datasource%%22:%%22PFB2D5CACEC34D62E%%22,%%22queries%%22:%%5B%%7B%%22refId%%22:%%22A%%22,%%22expr%%22:%%22%%7Bnamespace%%3D%%5C%%22%s%%5C%%22%%7D%%22,%%22queryType%%22:%%22range%%22,%%22datasource%%22:%%7B%%22type%%22:%%22loki%%22,%%22uid%%22:%%22PFB2D5CACEC34D62E%%22%%7D,%%22editorMode%%22:%%22code%%22%%7D%%5D,%%22range%%22:%%7B%%22from%%22:%%22now-1h%%22,%%22to%%22:%%22now%%22%%7D%%7D", installation.ID) |
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.
Rather than mucking around with adding the Grafana API for shortlinking I opted to just use the full link, and then leverage md to "beautify" it
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.
Looks solid! Does it make sense to include log links in any of the failure messages that are sent out?
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.
I'm swaying on the side of "no" but open to adding if you think it's worth it.
Up to this point, the installation hasn't actually been created, so any error would be strictly logged inside Matterwick (only Cloud team can see logs because Cloud Core) - or the Provisioner. Staff can see Provisioner logs in the test account, but I don't know if there's any error that someone could find in Provisioner and then resolve without asking us for help, so I don't think it gets us anywhere
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.
Yeah, likely they won't see logs (it would have to be an issue where the server is crashing and never goes stable).
Let's just ship this and we can tackle error logs later in a follow-up.
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.
LGTM
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.
LGTM, awesome :)
Summary
This PR aims to reduce friction around developers finding the installation ID in order to be able to find logs for a spinwick. This PR makes it so that the log link is echo'd into the comment where Matterwick shares the credentials, and it includes the installation ID as well.
Ticket Link
https://mattermost.atlassian.net/browse/CLD-6341
Release Note