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 all 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
7 changes: 6 additions & 1 deletion cmd/ltctl/report.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ func RunGenerateReportCmdF(cmd *cobra.Command, args []string) error {
}

if promURL == "" {
fmt.Printf("Flag --prometheus-url is not set. Defaulting to the deployment's Prometheus server...")
fmt.Println("Flag --prometheus-url is not set. Defaulting to the deployment's Prometheus server...")
config, err := getConfig(cmd)
if err != nil {
return err
Expand All @@ -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("no active deployment found, use the `--prometheus-url` flag if you have a local or manually deployed Prometheus server running")
}

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

Expand Down
4 changes: 2 additions & 2 deletions deployment/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ type Config struct {
Report report.Config
// Directory under which the .terraform directory and state files are managed.
// It will be created if it does not exist
TerraformStateDir string `default:"/var/lib/mattermost-load-test-ng" validate:"notempty"`
TerraformStateDir string `default:"./ltstate" validate:"notempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Fine with the change, but is this somehow related to the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @agnivade ! :D
I did this so we can reach !output.HasMetrics() in report.go.
As far as I understand, we want to change the default directory, because when deployer.json is not configured it takes default path and we get a 'no permission' error instead no deployment

This is what @agarciamontoro mentioned in this comment.

Copy link
Member

Choose a reason for hiding this comment

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

@agnivade: Not the original goal, but to get to the correct state we wanted, we need it.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, understood!

// URI of an S3 bucket whose contents are copied to the bucket created in the deployment
S3BucketDumpURI string `default:"" validate:"s3uri"`
// An optional URI to a MM server database dump file
Expand Down 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
3 changes: 3 additions & 0 deletions deployment/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,9 @@ func TestClusterSubnetIDs(t *testing.T) {

require.NotNil(t, cfg.ClusterSubnetIDs.Redis)
require.Len(t, cfg.ClusterSubnetIDs.Redis, 0)

require.NotNil(t, cfg.TerraformStateDir)
require.Equal(t, "./ltstate", cfg.TerraformStateDir)
})

t.Run("String() of default values", func(t *testing.T) {
Expand Down
Loading