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

Misleading error when generating a report with no deployment #846

Merged
merged 6 commits into from
Nov 18, 2024
Merged

Misleading error when generating a report with no deployment #846

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
690ac4a
Fix inconsistent error while generating report with no deployment
kczpl Nov 10, 2024
374433e
Merge branch 'mattermost:master' into fix/no-deployment-misleading-error
kczpl Nov 11, 2024
a4a3c29
Improve error message while no active deployment found and change
kczpl Nov 12, 2024
513551c
Remove unnecessary new line on no metrics error in report generation
kczpl Nov 15, 2024
8bec619
Remove comment in config.go
kczpl Nov 15, 2024
cc5a7f6
Update no deployment message in report generation
kczpl Nov 15, 2024
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
5 changes: 5 additions & 0 deletions cmd/ltctl/report.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,11 @@ func RunGenerateReportCmdF(cmd *cobra.Command, args []string) error {
if err != nil {
return fmt.Errorf("could not parse output: %w", err)
}

if !output.HasMetrics() {
return fmt.Errorf("the output has no metrics, no active deployment found")
Copy link
Member

Choose a reason for hiding this comment

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

Here we can return something slightly more useful to the user, in the lines of:

Suggested change
return fmt.Errorf("the output has no metrics, no active deployment found")
return fmt.Errorf("no active deployment found, use the --prometheus-url flag if you have a local or manually deployed Prometheus server running")

Copy link
Contributor Author

@kczpl kczpl Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

sure thing! Added that with an extra \n for better readability.

Flag --prometheus-url is not set. Defaulting to the deployment's Prometheus server...
Error: no active deployment found, use the `--prometheus-url` flag.
If you have a local or manually deployed Prometheus server running
exit status 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If no deployer.json is configured at all, there is another error that is returned earlier. I'm thinking that we can fix this by setting the default of TerraformStateDir as ./ltstatus (I don't really know why I didn't do that when I added it, to be honest). The error is the following:

I get now - I thought it was intentional and that the error occurred because I had no state provided. I have changed the default as suggested and added a small test.

It seems we're missing a newline when we return the Flag --prometheus-url is not set. Defaulting to the deployment's Prometheus server... error, so the new error is a bit buried. Can you add it?

Not a problem! I agree it's not user-friendly, and I've fixed that

}

promURL = "http://" + output.MetricsServer.PublicIP + ":9090"
}

Expand Down
2 changes: 1 addition & 1 deletion deployment/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ type TerraformDBSettings struct {
// and provisioned.
type ExternalDBSettings struct {
// Mattermost database driver
DriverName string `default:"" validate:"oneof:{mysql, postgres, cockroach}"`
DriverName string `default:"postgres" validate:"oneof:{mysql, postgres, cockroach}"`
// DSN to connect to the database
DataSource string `default:""`
// DSN to connect to the database replicas
Expand Down
Loading