-
Notifications
You must be signed in to change notification settings - Fork 119
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
Dump installed objects for a given package #670
Conversation
So far, it exports the following objects when run for the system package:
|
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
ff60144
to
b01b737
Compare
Getting the objects starting from the data streams is not reliable, they may not exist if no data has been ingested. Refactored to start discovering from the index templates. For the apache package these objects are exported now:
|
d805022
to
38d81a3
Compare
Opening for review. @DJRickyB @tomcallahan it'd be nice if you could give it a try. You can find sample files exported by this new subcommand in https://github.com/elastic/elastic-package/tree/bbbb70a54495b8b32f42846797733ccdc74ed54d/internal/export/testdata/apache-export-all |
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.
First review round, mostly focused on API.
cmd/export.go
Outdated
@@ -33,12 +38,25 @@ func setupExportCommand() *cobraext.Command { | |||
exportDashboardCmd.Flags().StringSliceP(cobraext.DashboardIDsFlagName, "d", nil, cobraext.DashboardIDsFlagDescription) | |||
exportDashboardCmd.Flags().Bool(cobraext.TLSSkipVerifyFlagName, false, cobraext.TLSSkipVerifyFlagDescription) | |||
|
|||
exportInstalledObjectsCmd := &cobra.Command{ | |||
Use: "installed-objects", | |||
Short: "Export installed Elasticsearch objects", |
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.
Just double-checking: there aren't any Kibana installed object, right?
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.
The command only exports the installed Elasticsearch objects now, as they are the relevant ones for performance testing.
We could consider at least index patterns and dashboards as things to export too. I haven't thought a lot about what would we do if we also want to export them in the same fashion. One option is to just add it here and rephrase this. On the other hand we already have a command to export dashboards, and maybe we could have another one to export index patterns, or add a flag to export dashboards to export the used pattern too.
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.
Same story, if this is a low-hanging fruit, then please consider 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.
Not such a low hanging fruit as the source of data would be a different API, and not needed at the moment, I would leave it for a future improvement.
cmd/export.go
Outdated
RunE: exportInstalledObjectsCmd, | ||
} | ||
exportInstalledObjectsCmd.Flags().StringP(cobraext.ExportPackageFlagName, "p", "", cobraext.ExportPackageFlagDescription) | ||
exportInstalledObjectsCmd.MarkFlagRequired(cobraext.ExportPackageFlagName) |
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.
There will be inconsistency with the other export
command that uses the current work dir to read the package context. Maybe this command should also follow that rule?
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.
With the approach here you can export the objects without needing the source of the package, I think this will be useful for the current use case, and it actually doesn't need anything about the current package context.
If we want to make all export
subcommands to be consistent on this I think I would prefer to add both options to both subcommands (i.e. both work with the package context and with the package flag) :)
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.
If we want to make all export subcommands to be consistent on this I think I would prefer to add both options to both subcommands (i.e. both work with the package context and with the package flag) :)
Yes, I was looking at low hanging fruits :) I will leave you to make the decision.
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 have added a test so this continues being a low hanging fruit in the future, but I am not taking it by now :)
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 implementation looks good, not sure if we arguments to discuss the API.
cmd/export.go
Outdated
RunE: exportInstalledObjectsCmd, | ||
} | ||
exportInstalledObjectsCmd.Flags().StringP(cobraext.ExportPackageFlagName, "p", "", cobraext.ExportPackageFlagDescription) | ||
exportInstalledObjectsCmd.MarkFlagRequired(cobraext.ExportPackageFlagName) |
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.
If we want to make all export subcommands to be consistent on this I think I would prefer to add both options to both subcommands (i.e. both work with the package context and with the package flag) :)
Yes, I was looking at low hanging fruits :) I will leave you to make the decision.
cmd/export.go
Outdated
@@ -33,12 +38,25 @@ func setupExportCommand() *cobraext.Command { | |||
exportDashboardCmd.Flags().StringSliceP(cobraext.DashboardIDsFlagName, "d", nil, cobraext.DashboardIDsFlagDescription) | |||
exportDashboardCmd.Flags().Bool(cobraext.TLSSkipVerifyFlagName, false, cobraext.TLSSkipVerifyFlagDescription) | |||
|
|||
exportInstalledObjectsCmd := &cobra.Command{ | |||
Use: "installed-objects", | |||
Short: "Export installed Elasticsearch objects", |
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.
Same story, if this is a low-hanging fruit, then please consider it.
@mtojek moved to a new I would be tempted to make this the only action of this |
@@ -12,3 +12,13 @@ func TrimStringSlice(slice []string) { | |||
slice[iterator] = strings.TrimSpace(item) | |||
} | |||
} | |||
|
|||
// StringSliceContains checks if the slice contains the given string. | |||
func StringSliceContains(slice []string, s string) bool { |
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.
nit: I'm wondering to how many places in the source code it can applied :)
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.
We can look for them in a follow up, and perhaps if we wait to https://tip.golang.org/doc/go1.18#generics we can make it generic and apply it even more extensively :)
return errors.Wrap(err, "dump failed") | ||
} | ||
if n == 0 { | ||
cmd.Printf("No objects were dumped for package %s, is it installed?", packageName) |
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.
nit: do you think we can check if the package is installed apriori? I hope there is a fleet API to make sure it is.
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.
Maybe, but this way this tool can also discover letfovers after uninstalling a package :) Also adding a check against the fleet API adds an additional dependency. As the command is now, it only needs access to Elasticsearch.
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.
Maybe, but this way this tool can also discover letfovers after uninstalling a package
It's a discussion whether it's a bug or feature :) We can keep it as is for now.
cmd/dump.go
Outdated
Long: dumpInstalledObjectsLongDescription, | ||
RunE: dumpInstalledObjectsCmd, | ||
} | ||
dumpInstalledObjectsCmd.Flags().StringP(cobraext.DumpPackageFlagName, "p", "", cobraext.DumpPackageFlagDescription) |
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.
Shouldn't -p
and -o
be a command
level instead of subcommand
?
cmd/dump.go
Outdated
Long: dumpInstalledObjectsLongDescription, | ||
RunE: dumpInstalledObjectsCmd, | ||
} | ||
dumpInstalledObjectsCmd.Flags().StringP(cobraext.DumpPackageFlagName, "p", "", cobraext.DumpPackageFlagDescription) |
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 hope it isn't possible to install multiple package versions at once :)
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.
Well, if some day this is possible, I guess that the worst thing that can happen with current implementation is that the objects of all installed versions will be collected, what I would find fine. We can add an optional version filter if this day arrives.
cmd/dump.go
Outdated
} | ||
dumpInstalledObjectsCmd.Flags().StringP(cobraext.DumpPackageFlagName, "p", "", cobraext.DumpPackageFlagDescription) | ||
dumpInstalledObjectsCmd.MarkFlagRequired(cobraext.DumpPackageFlagName) | ||
dumpInstalledObjectsCmd.Flags().StringP(cobraext.DumpOutputFlagName, "o", "output", cobraext.DumpOutputFlagDescription) |
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.
Let's discuss possible dump
use cases:
elastic-package dump installed-objects -p hello-world
elastic-package dump assets -p hello-world
elastic-package dump assets -p hello-world
elastic-package dump indices -p hello-world
elastic-package dump sample-event -p hello-world
WDYT about storing data in a structured way, for example output
-dir can be replaced with package-dump
, which may contain installed-objects
, indices
, etc.
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.
Changing the default from output
to package-dump
sgtm, but I think I would keep the internal structure flat. If we dump additional resources as indices or sample events at some moment we can store them under package-dump/events
and package-dump/indices
and so on.
…default output directory
I have found a couple of places where there can be nested pipelines, adding some logic to collect them too... |
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
return errors.Wrap(err, "dump failed") | ||
} | ||
if n == 0 { | ||
cmd.Printf("No objects were dumped for package %s, is it installed?", packageName) |
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.
Maybe, but this way this tool can also discover letfovers after uninstalling a package
It's a discussion whether it's a bug or feature :) We can keep it as is for now.
@DJRickyB @tomcallahan did you have the chance of trying this new command to dump ES objects related to packages? |
Hello! I'll get into this today |
This works great, thank you very much. Interesting, the departure from the existing |
@DJRickyB thanks for trying it! Yep, both commands are similar, but we decided to leave the On the other hand |
Thank you for the context, makes total sense |
Add the
elastic-package dump installed-objects
to dump the objects installed for a package in Elasticsearch.This command is intended to be used as an exploratory tool, to be able to analyze the resources associated to a given package.
For testing, a helper is added that starts a client for a mocked server with recorded responses.
To try it:
Fixes #600