Install additional plugins #65
Conversation
ajhewett
commented
Oct 24, 2016
ajhewett
commented
- Option to install additional plugins
- Default parameters install no additional plugins
- Added option to uiDefinition
- Updated README
|
Rebased against master. |
|
Thanks for opening this new PR @ajhewett; reviewing now |
There was a problem hiding this comment.
I've left some comments on the PR; thank you for persevering, despite my pedantry 😆
If you want to add any additional commits and then squash, you can add the commits to your branch then do an interactive rebase, specifying the number of commits to rebase. For example, to rebase (and squash) the last 4 commits on your branch
git rebase -i HEAD~4This will open up you editor where you can choose the action for each commit. Change all but the top commit to squash, write the combined commit message. Once you have done this, your local branch will be different to your branch on github, so you would need to update github using git push -f to force push.
| "toolTip": "Additional elasticsearch plugins to install. Each plugin must be separated by a semicolon. e.g. delete-by-query;lmenezes/elasticsearch-kopf/2.0", | ||
| "constraints": { | ||
| "required": false | ||
| } |
There was a problem hiding this comment.
I'm not keen on including this option in the UI definition as an input parameter; we can simply set the output of esAdditionalPlugins in the UiDefinition to "".
My concern with including this in the inputs is that I consider this to be an expert level feature because you need to know
- which additional plugins you want (and their versions) for the Elasticsearch version
- that by installing them in the cluster, are aware of the caveats, lack of official support and potential instability for running third party plugins on the cluster.
Furthermore, it's another input to consider and our intention with the UI definition is to keep things as easy as possible for the first time user in getting an Elasticsearch cluster up and running on Azure.
Finally, it encourages others to start with the UI definition and then go further and build upon the deployment template (which we're looking to do a blog series on).
/cc @elastic/microsoft @cjgeode Thoughts?
There was a problem hiding this comment.
TBH I only added because I thought it was necessary to make the uiDefinition outputs work.
Have updated the pull request.
| @@ -1015,6 +1025,7 @@ | |||
| "azureCloudPlugin": "[steps('clusterSettingsStep').azureCloudStorageAccountConfiguration.azureCloudPlugin]", | |||
| "azureCloudStorageAccountName": "[steps('clusterSettingsStep').azureCloudStorageAccountConfiguration.azureCloudStorageAccountName]", | |||
| "azureCloudStorageAccountKey": "[steps('clusterSettingsStep').azureCloudStorageAccountConfiguration.azureCloudStorageAccountKey]", | |||
| "esAdditionalPlugins": "[steps('clusterSettingsStep').esAdditionalPlugins]", | |||
There was a problem hiding this comment.
See comment above; we can simply set this to ""
There was a problem hiding this comment.
Have updated the pull request.
| "type": "string", | ||
| "defaultValue": "", | ||
| "metadata": { | ||
| "description": "Install additional Plugins. Each plugin separated by a semicolon. e.g. delete-by-query;lmenezes/elasticsearch-kopf/2.0" |
There was a problem hiding this comment.
minor nitpick - could we use some other plugins as examples? delete-by-query is deprecated in 2.x and replace by delete-by-query API, and Kopf is a third party plugin. For example,
There was a problem hiding this comment.
Sounds reasonable.
Have updated the pull request.
| install_additional_plugins() | ||
| { | ||
| log "[install_additional_plugins] Installing additional plugins" | ||
| for PLUGIN in $(echo $INSTALL_ADDITIONAL_PLUGINS | tr ";" "\n") |
There was a problem hiding this comment.
Thinking about the case where a user specifies an additional plugin here the same as one already installed e.g. Shield, Marvel, Azure Cloud, etc. Should we skip those when enumerating through the additional plugins?
There was a problem hiding this comment.
Updated pull request to skip plugins installed by other means: license shield watcher marvel-agent graph cloud-azure
ajhewett
force-pushed
the
additional-plugins-rebased
branch
from
d7c7ca9
to
d9ca7b4
Compare
* Option to install additional plugins * Default parameters install no additional plugins * Updated README * Integrate review comments * Use official plugins in examples * Skip plugins installed by other means
ajhewett
force-pushed
the
additional-plugins-rebased
branch
from
4b4d083
to
8c4074a
Compare
|
@russcam integrated review comments and squashed to a single commit. |
|
Thanks again for submitting this @ajhewett 👍 Would you mind pinging me an email with the best email address to contact you on (you can find mind in the git log)? |
