-
Notifications
You must be signed in to change notification settings - Fork 9
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
SE-100: don't dump when instance is not found #173
base: main
Are you sure you want to change the base?
Conversation
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 with comments
@@ -44,17 +45,18 @@ func TestExportImport(t *testing.T) { | |||
if err := g.Wait(); err != nil { | |||
t.Fatal(err) | |||
} | |||
time.Sleep(30 * time.Second) |
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.
It would be great to have comment there what we are waiting here.
if strings.Contains(stderr, "Failed to create a dump. No data was found") { | ||
// If pmm-dump returns this error, it also means that the dashboard selector parsing was successful | ||
return | ||
} |
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.
It looks like relying on exit code may be a better option rather than error text.
https://perconadev.atlassian.net/browse/SE-100
In this PR,
pmm-dump
will fail during export when it can't find any data, and it will display the messageFailed to create a dump. No data was found
.If the
--instance
flag was provided, it will issue a warning before failing, with the messageIt seems that data about instances specified in the `--instance` option does not exist in the PMM server.