elastic / elastic/elastic-package

Explicitly validate variables in test configurations

Open
#3,365 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
72
Forks
141
Avg merge
19h 42m
Merged PRs (30d)
55

Description

Variables in system and policy tests are only used in policies if they are defined in package manifests. The rest of variables are silently ignored.

This can be convenient, as variables are defined and used at several levels, but has the problem of silently hiding problems caused by typos in test definitions. A developer may be intending to set a variable, use a misspelled name and then the test case wouldn't cover what the developer intended.

Some cases of this were fixed in https://github.com/elastic/integrations/pull/17656 and https://github.com/elastic/integrations/pull/17695, but there could be more cases of variables that are set in the config but not used.

Another issue is that as mentioned, there are different levels where variables can be set: package, input and data stream level. At the moment test configurations allow only two levels, a top-level `vars`, and another one under `data_stream`. Though both sets of variables are used in all levels.
elastic-package should also be more explicit about this, I think the current approach would be one of these:
* Variables are only set in test configurations at one level, `vars`, and they are used at any of the configuration levels. `data_stream.vars` is kept to avoid breaking changes, and these variables are appended to `vars`. I think this is de facto what is mostly done now, and this is what is more convenient for developers. For validations elastic-package would need to check that all variables set in test are defined at least at one of the three levels.
* Variables have to be set in test configurations at the same level where they are intended to be used. This would be more explicit, but not sure if so useful. This would be useful in cases where a single test case defines multiple data streams in the same policy, but in principle we are not supporting this. To support this we would need to ensure that `vars`, `input.vars` and `data_stream.vars` can be defined, and are used only at their intended levels. For validation this would mean that each group of variables is defined in the specific property in the manifests. If we go in this direction we probably need to fix the tests in many packages.

References:
* Variables are set according to definitions in [`SetKibanaVariables`](https://github.com/elastic/elastic-package/blob/b49f20a2495d63666f7046809ccd1ce9df54f93c/internal/kibana/policies.go#L257).
* `SetKibanaVariables` is used at the different levels of variables in [BuildIntegrationPackagePolicy](https://github.com/elastic/elastic-package/blob/b49f20a2495d63666f7046809ccd1ce9df54f93c/internal/kibana/packagepolicy.go#L17) and [BuildInputPackagePolicy](https://github.com/elastic/elastic-package/blob/b49f20a2495d63666f7046809ccd1ce9df54f93c/internal/kibana/packagepolicy.go#L144).

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.