Scan stable plugins for named components upon install#92528
Scan stable plugins for named components upon install#92528pgomulka merged 37 commits intoelastic:mainfrom
Conversation
|
Hi @pgomulka, I've created a changelog YAML for you. |
…plugin_install_scanning
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
rjernst
left a comment
There was a problem hiding this comment.
I have a general comment about how testing is done. I think we should avoid mocks (mockito), and then we shouldn't need to have instances of the utility classes or pass them into the actions.
|
|
||
| @SuppressForbidden(reason = "sets java.io.tmpdir") | ||
| public InstallPluginActionTests(FileSystem fs, Function<String, Path> temp) { | ||
| assert "false".equals(System.getProperty("tests.security.manager")) : "-Dtests.security.manager=false has to be set"; |
There was a problem hiding this comment.
I think using just assert looks better. this is not a real assertion but a hint when running from IDE
with assumeTrue it would look confusing
assumeTrue("-Dtests.security.manager=false has to be set", "false".equals(System.getProperty("tests.security.manager"));
| public class ClassReadersProvider { | ||
| private static final String MODULE_INFO = "module-info.class"; | ||
|
|
||
| public List<ClassReader> ofClassPath() throws IOException { |
There was a problem hiding this comment.
What in this class requires some of these methods to be instance methods? I don't see a ctor or any instance member vars. I suspect this was for mocking; see my other comment. Note that it is especially odd to make some methods here instance methods, but still also have static utility methods.
There was a problem hiding this comment.
yes, this was only made an instance to allow mocking this
| import org.elasticsearch.test.PosixPermissionsResetter; | ||
| import org.junit.After; | ||
| import org.junit.Before; | ||
| import org.mockito.Mockito; |
There was a problem hiding this comment.
I don't think we should use mocks here. The rest of the tests here use fakes (we call them mocks, but they are custom mocks/filesystem impls). Ultimately what we are trying to test is just a couple cases, only one or two that need an actual components file, to test the integration.
There was a problem hiding this comment.
the only way to avoid the use of mockito is to use a "fake jar with no content" so that it relies on nothing being scanned, and nothing being written. (updated the PR)
The classes are being used and file is being created. And this is what that test is mainly trying to show, so perhaps it is ok.
| if (hasNamedComponentFile) { | ||
| PluginTestUtil.writeNamedComponentsFile(structure, namedComponentsJSON()); | ||
| } | ||
| writeJar(structure.resolve("plugin.jar"));// empty jar so that there is nothing to scan |
There was a problem hiding this comment.
should we want something in the jar so that the named component file is not empty?
| installPlugins(List.of(stablePluginZip), env.v1()); | ||
|
|
||
| assertPlugin("stable1", pluginDir, env.v2()); | ||
| // the test was using a jar with no classes, so the scanner did not find anything, but this is fine. that logic is tested elsewhere |
There was a problem hiding this comment.
I think we care about slightly more than that. Is there a legitimate case for an empty file? I think we want to see that the generation was called and a real file was created, with at least one actual component. I would do that in writeStablePlugin using the InMemoryJavaCompiler.
There was a problem hiding this comment.
fair point, InMemoryJavaCompiler might help with test here and it helped me realise that plugin-cli has to have a dependency on plugin-api. I incorrectly assumed that it might be shipped with plugin, but it will not. Plugins only have 'implementation/compileOnly' scope on it.
| } | ||
|
|
||
| // pkg private for testing | ||
| public void setNamedComponentScanner(NamedComponentScanner scanner) { |
There was a problem hiding this comment.
This seems leftover, it is no longer used?
stable plugins not build with ES's gradle plugin will not have named_components.json file.
To allow these plugins to expose their named components, a scan can be performed upon install.
relates #88980