Update packaging tests to work with meta plugins #28336
Conversation
tlrx
added
>test
| local user="$ESPLUGIN_COMMAND_USER" | ||
| local group="$ESPLUGIN_COMMAND_USER" | ||
|
|
||
| assert_file_exist "$ESPLUGINS/$name/meta-plugin-descriptor.properties" f $user $group 775 |
There was a problem hiding this comment.
I don't understand why we need such checks. The installer will not succeed if the plugin zip is malformed (ie missing descriptor file) so why make these tests susceptible to breaks when we change the internal format? For example, I have considered making plugins put jar files under a lib dir, which would again break these tests, but for no good reason.
There was a problem hiding this comment.
I'm not sure we need this one either. I get wanting to have some assertion early after installing the plugin but I think it is enough to make sure that the directory exists.
My take on the reason we assert that jars and stuff are actually created:
- We assert the owner and permissions on the JAR in the real-est way possible. If any of that is important then the assertion is important.
- We want to do some limited sanity check that the plugin is installed correctly. We could execute some API call against the plugin to prove that it is loaded properly but this is brittle in that it relies on the plugin's behavior. And it requires a running Elasticsearch. What we have here is fast but brittle for all the reasons Ryan mentioned.
I don't know the right solution. I think not asserting anything at all after installing the plugin isn't great. Even installing the plugin, starting Elasticsearch, and listing the plugins isn't really enough to ensure the plugin isn't borked. Asserting the jars was a simple thing we could do at the time but a thing I really would like to rethink.
There was a problem hiding this comment.
I used to add tests for files and directories that are accessed by Elasticsearch at runtime. This is not the case of the meta-plugin-descriptor.properties so I agree it can be removed.
I still think that we need some basic assertions after installing a plugin (meta or not) in the packaging tests. The most important to me are checking the permissions and owerships of plugin's bin, config, descriptor and security policy files on all linux-supported platforms. We had enough issues with file permissions/owerships on various operating systems and I don't think we're confident enough to remove these checks. I also think they are not very costly to maintain compared to the safety belt they provide.
For the plugin's jars and internal directory structure I agree it makes less sense to test them in detail. We should have better integration tests for each plugin instead to ensure the plugin isn't borked.
There was a problem hiding this comment.
The most important to me are checking the permissions and owerships of plugin's bin, config, descriptor and security policy files on all linux-supported platforms
But we already have tests for this. By having these checks here, we are re-testing the elasticsearch-plugin install command behavior for every official plugin. That duplication is what makes this a pain to maintain.
Note this PR is fine on its own, I only wanted to suggest simplify this testing since it was slightly touched here.
There was a problem hiding this comment.
Note this PR is fine on its own, I only wanted to suggest simplify this testing since it was slightly touched here.
Good, let's move forward then and we could revisit the duplication in a potential follow up pull request.
| local user="$ESPLUGIN_COMMAND_USER" | ||
| local group="$ESPLUGIN_COMMAND_USER" | ||
|
|
||
| assert_file_exist "$ESPLUGINS/$name/meta-plugin-descriptor.properties" f $user $group 775 |
There was a problem hiding this comment.
I'm not sure we need this one either. I get wanting to have some assertion early after installing the plugin but I think it is enough to make sure that the directory exists.
My take on the reason we assert that jars and stuff are actually created:
- We assert the owner and permissions on the JAR in the real-est way possible. If any of that is important then the assertion is important.
- We want to do some limited sanity check that the plugin is installed correctly. We could execute some API call against the plugin to prove that it is loaded properly but this is brittle in that it relies on the plugin's behavior. And it requires a running Elasticsearch. What we have here is fast but brittle for all the reasons Ryan mentioned.
I don't know the right solution. I think not asserting anything at all after installing the plugin isn't great. Even installing the plugin, starting Elasticsearch, and listing the plugins isn't really enough to ensure the plugin isn't borked. Asserting the jars was a simple thing we could do at the time but a thing I really would like to rethink.
| # $1 - the plugin name | ||
| # $@ - all remaining arguments are jars that must exist in the plugin's | ||
| # installation directory | ||
| install_meta_plugin() { |
There was a problem hiding this comment.
I don't see where this is used. Am I missing something?
There was a problem hiding this comment.
This is not used in this pull request but we need it for our meta plugins and I think this is where it should be placed.
tlrx
force-pushed
the
meta-plugin-packaging-tests
branch
from
81334cb to
4b8130b
Compare
The current install_plugin() does not play well with meta plugins because it always checks for the plugin's descriptor file. This commit changes the install_plugin() so that it only runs the install plugin command and lets the caller verify that the required files are correctly installed. It also adds a install_meta_plugin() function to install meta plugins.
The current install_plugin() does not play well with meta plugins because it always checks for the plugin's descriptor file. This commit changes the install_plugin() so that it only runs the install plugin command and lets the caller verify that the required files are correctly installed. It also adds a install_meta_plugin() function to install meta plugins.
* es/master: [Docs] Fix explanation for `from` and `size` example (#28320) Adapt bwc version after backport #28358 Always return the after_key in composite aggregation response (#28358) Adds test name to MockPageCacheRecycler exception (#28359) Adds a note in the `terms` aggregation docs regarding pagination (#28360) [Test] Fix DiscoveryNodesTests.testDeltas() (#28361) Update packaging tests to work with meta plugins (#28336) Remove Painless Type from MethodWriter in favor of Java Class. (#28346) [Doc] Fixs typo in reverse-nested-aggregation.asciidoc (#28348) Reindex: Shore up rethrottle test Only assert single commit iff index created on 6.2 isHeldByCurrentThread should return primitive bool [Docs] Clarify `html` encoder in highlighting.asciidoc (#27766) Fix GeoDistance query example (#28355) Settings: Introduce settings updater for a list of settings (#28338) Adapt bwc version after backport #28310
* es/6.x: [Docs] Fix explanation for `from` and `size` example (#28320) Adapt bwc version after backport #28358 Always return the after_key in composite aggregation response (#28358) Adds a note in the `terms` aggregation docs regarding pagination (#28360) Update packaging tests to work with meta plugins (#28336) Remove Painless Type from MethodWriter in favor of Java Class. (#28346) [Doc] Fixs typo in reverse-nested-aggregation.asciidoc (#28348) [Docs] Fixed Indices information breaking changes (#27914) Reindex: Shore up rethrottle test isHeldByCurrentThread should return primitive bool [Docs] Clarify `html` encoder in highlighting.asciidoc (#27766) Only assert single commit iff index created on 6.2 Deprecate the `update_all_types` option. (#28284) Fix GeoDistance query example Settings: Introduce settings updater for a list of settings (#28338) [Docs] Fix asciidoc style in composite agg docs Adapt bwc version after backport #28310 Adds the ability to specify a format on composite date_histogram source (#28310)
* master: (23 commits) Update Netty to 4.1.16.Final (elastic#28345) Fix peer recovery flushing loop (elastic#28350) REST high-level client: add support for exists alias (elastic#28332) REST high-level client: move to POST when calling API to retrieve which support request body (elastic#28342) Add Indices Aliases API to the high level REST client (elastic#27876) Java Api clean up: remove deprecated `isShardsAcked` (elastic#28311) [Docs] Fix explanation for `from` and `size` example (elastic#28320) Adapt bwc version after backport elastic#28358 Always return the after_key in composite aggregation response (elastic#28358) Adds test name to MockPageCacheRecycler exception (elastic#28359) Adds a note in the `terms` aggregation docs regarding pagination (elastic#28360) [Test] Fix DiscoveryNodesTests.testDeltas() (elastic#28361) Update packaging tests to work with meta plugins (elastic#28336) Remove Painless Type from MethodWriter in favor of Java Class. (elastic#28346) [Doc] Fixs typo in reverse-nested-aggregation.asciidoc (elastic#28348) Reindex: Shore up rethrottle test Only assert single commit iff index created on 6.2 isHeldByCurrentThread should return primitive bool [Docs] Clarify `html` encoder in highlighting.asciidoc (elastic#27766) Fix GeoDistance query example (elastic#28355) ...
The current
install_plugin()does not play well with meta plugins because it always checks for the plugin's descriptor file.This commit changes the
install_plugin()so that it only runs the install plugin command and lets the caller verify that the required files are correctly installed. It also adds ainstall_meta_plugin()function to install meta plugins.