-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Convert repository plugins to modules #81870
Convert repository plugins to modules #81870
Conversation
Pinging @elastic/es-distributed (Team:Distributed) |
Pinging @elastic/es-delivery (Team:Delivery) |
Pinging @elastic/es-core-infra (Team:Core/Infra) |
Hi @pugnascotia, I've created a changelog YAML for you. |
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.
Couple of minor comments but otherwise LGTM. Looks like we have some minor packaging test fallout but I don't think there are any fundamental issues with the implementation.
@@ -115,7 +116,14 @@ public void execute() throws Exception { | |||
PluginChanges getPluginChanges(PluginsConfig pluginsConfig, Optional<PluginsConfig> cachedPluginsConfig) throws PluginSyncException { | |||
final List<PluginInfo> existingPlugins = getExistingPlugins(); | |||
|
|||
final List<PluginDescriptor> pluginsThatShouldExist = pluginsConfig.getPlugins(); | |||
// For plugins that have migrated to modules, in order to help transition it's OK to still specify these plugins |
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 that's the case should we even bother filtering here? Wouldn't it be better to just delegate on and get the warning message which would inform the user to remove these from their plugin config? As it is now we'd be silencing that message, no?
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 problem with delegating is that in this situation, the config file might say "repository-s3" should be installed, but actually we need to remove that plugin because it's now a module. So we need to change the "this plugin should be present" into a "this plugin should be removed". As a result, we don't reach the logic in InstallPluginAction
that logs the warning about modularized plugins. However I realised that this implementation wasn't printing that warning at all when using a config file, so I've corrected that.
if (PLUGINS_CONVERTED_TO_MODULES.contains(pluginId)) { | ||
// This deliberately does not throw an exception in order to avoid failing automation that relies on installing this | ||
// plugin during deployment. | ||
terminal.errorPrintln( |
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 wonder if there are additionally any concerns with sending this to stderrr? Would stdout similarly break stuff depending how folks are consuming the output?
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 thing is, the output from the CLI tools is completely unstructured. The only contract is the exit code of the tool, which is zero when installing or removing a modularized plugin. We can't reasonably be expected never to change the output of the CLI when it has no documented output format.
docs/plugins/repository.asciidoc
Outdated
<<repository-s3,S3 Repository>>:: | ||
|
||
The S3 repository plugin adds support for using S3 as a repository. | ||
NOTE: The S3, GCS and Azure repository plugins are now included in the {es} |
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.
Since they're no longer "plugins" I'd reword this to say
NOTE: Support for S3, GCS and Azure repository is now bundled in {es} by default
I'd also rename the section "Core repository plugins" to "Official repository plugins".
@elastic/es-docs thoughts?
@elasticmachine retest this please |
The last broken links from the Kibana link service are fixed by elastic/kibana#121834. Once this PR is ready to go except for the broken links, it can be merged with the failing doc check and the Kibana PR can be retested & merged. |
@elasticmachine run elasticsearch-ci/cloud-deploy |
This PR requires changes from Cloud before we can merge. |
…nto repository-plugins-to-modules
Closes elastic#81652. Convert the `repository-azure`, `repository-gcs` and `repository-s3` plugins into modules, so that they are always included in the Elasticsearch distribution. Also change plugin installation, removal and syncing so that attempting to add or remove these plugins still succeeds but is now a no-op.
The repository plugins for S3, Azure, and GCS are being moved into Elasticsearch as modules. This PR updates the Kibana links to point to the Elasticsearch guide rather than the Plugin guide. Related to elastic/elasticsearch#81870
Backport of #81870. Closes #81652. Convert the `repository-azure`, `repository-gcs` and `repository-s3` plugins into modules, so that they are always included in the Elasticsearch distribution. Also change plugin installation, removal and syncing so that attempting to add or remove these plugins still succeeds but is now a no-op.
The repository plugins for S3, Azure, and GCS are being moved into Elasticsearch as modules. This PR updates the Kibana links to point to the Elasticsearch guide rather than the Plugin guide. Related to elastic/elasticsearch#81870 Co-authored-by: debadair <debadair@elastic.co>
With elastic/elasticsearch#81870, we migrated docs for the Azure, GCS, and S3 snapshot repositories to the ES guide from the ES plugin docs. However, this change only applies to Elastic 8.0+. These attributes let the Cloud docs link to the "plugin" version of the docs when referencing 7.x and 6.x Elastic deployments without breaking links. Relates to https://github.com/elastic/cloud/pull/95621 and #2312
With elastic/elasticsearch#81870, the Azure, GCS, and AWS snapshot repository types have built-in support in Elasticsearch and no longer require plugins in 8.0+. This PR updates step one of the **Register Repository** wizard to: - Include Azure, GCS, and AWS as default repository types - Tweak UI copy and links referring to repository plugins. Co-authored-by: Yulia Čech <6585477+yuliacech@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
#123768) With elastic/elasticsearch#81870, the Azure, GCS, and AWS snapshot repository types have built-in support in Elasticsearch and no longer require plugins in 8.0+. This PR updates step one of the **Register Repository** wizard to: - Include Azure, GCS, and AWS as default repository types - Tweak UI copy and links referring to repository plugins. Co-authored-by: Yulia Čech <6585477+yuliacech@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 1e57604) # Conflicts: # x-pack/plugins/snapshot_restore/server/routes/api/repositories.test.ts
With #81870, the Azure, GCS, and S3 repository types have separate, dedicated pages in the Elasticsearch guide. For consistency, this PR creates separate pages for the shared file system, read-only URL, and source-only repository types. Related changes: - Adds redirects to the plugins docs - Fixes a few breaking changes that refer to the Azure, GCS, and S3 repositories as plugins. Co-authored-by: Adam Locke <adam.locke@elastic.co>
With #81870, the Azure, GCS, and S3 repository types have separate, dedicated pages in the Elasticsearch guide. For consistency, this PR creates separate pages for the shared file system, read-only URL, and source-only repository types. Related changes: - Adds redirects to the plugins docs - Fixes a few breaking changes that refer to the Azure, GCS, and S3 repositories as plugins. Co-authored-by: Adam Locke <adam.locke@elastic.co> (cherry picked from commit cb6265f)
Closes #81652.
Convert the
repository-azure
,repository-gcs
andrepository-s3
plugins into modules, so that they are always included in the Elasticsearch distribution. Also change plugin installation, removal and syncing so that attempting to add or remove these plugins still succeeds but is now a no-op.