-
Notifications
You must be signed in to change notification settings - Fork 127
add command line flag for system test result dumps #2967
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
base: main
Are you sure you want to change the base?
Conversation
internal/cobraext/flags.go
Outdated
TestDumpPrefixFlagName = "dump" | ||
TestDumpPrefixFlagDescription = "prefix for system test dump file" | ||
|
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.
Adding just one flag it forces to always set a prefix to the user, but probably they often need just a default value for that.
What about adding two flags to manage this feature?
- One flag to enable or not dumping the documents (
--dump-docs
) without defining any path or prefix.- The default filename could be the same one used when the environment variable is set.
- Should this file be saved in the same location where
elastic-package
runs ? Take into account that this could change with the parameter-C
.elastic-package -C test/packages/parallel/nginx test system -v
- Another flag to set a custom prefix (
--dump-docs-prefix
) or even it could be set the whole path (--dump-docs-output
) leaving to the user the choice to where save the output file. I think I'd prefer the latter.
# enable dumping documents and using the default filename for the path
elastic-package test system -v --dump-docs-
# enable dumping documents and using a custom path
elastic-package test system -v --dump-docs --dump-docs-output /path/to/dump.json
Doing it like this, it would be similar on how coverage is set in elastic-package
:
--test-coverage
to enable the feature,--coverage-format
to choose which format is required.
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 think this comes down to a philosophical position; I think having a default location for things like this is a significant misfeature. IMO actions should require explicit requests. Including things like this in the config add complexity to the user experience and is also IMO a misfeature.
I'm not entirely sure what the gain is for users in adding this additional complexity.
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.
IMO I think most users (from my POV) would like just to download/dump the documents without really requiring setting any specific path or prefix. That's why I would say that the most common usage would be (not requiring to set any prefix there):
elastic-package test system -v --dump-docs
Moreover, doing it like this, it would enable this feature and it could be coded to be equivalent to set the ELASTIC_PACKAGE_TEST_DUMP_SCENARIO_DOCS
environment variable. So users setting the environment variable and setting that flag can expect the same exact behavior of elastic-package
.
For the other flag I proposed, I thought there could be users that require some specific known file, for instance they are running tests with different packages and they want those files to be easily identified. That's my reasoning to add that new --dump-docs-output
or --dump-docs-prefix
flag, so they can decide how to save those documents.
# allow a full path
elastic-package test system -v --dump-docs --dump-docs-output /path/store/documents.json
# or as you proposed adding a prefix and save them in the same location where elastic-package runs
elastic-package test system -v --dump-docs --dump-docs-prefix "package-test"
The concern I have of writing those new documents in the same location as elastic-package runs is that this could lead to users committing and pushing those files in the repository inadvertently.
Also fix case of failing tests without validation in the case that the dump fails.
💚 Build Succeeded
History
cc @efd6 |
Please take a look.