Allowing custom folder name for plugin installation #446

VachaShah · 2021-06-04T23:38:45Z

Signed-off-by: Vacha Shah [email protected]

Description

This PR allows plugin owners to specify a custom folder name for plugin cli installation and override the folder name. This custom folder name can be specified in opensearch_dashboards.json for the plugin. If the folder name is specified in the opensearch_dashboards.json, the plugin is installed using the custom folder name as is specified for the plugin, else the id of the plugin is used in kebab-case.

Testing

The unit tests have been added. Also tested with a plugin anomaly-detection-dashboards-plugin, I have added an attribute in opensearch_dashboards.json as follows:

{
  "id": "anomalyDetectionDashboards",
  "folderName": "customFolder",
  "version": "1.0.0.0-beta1",
  "opensearchDashboardsVersion": "1.0.0-beta1",
  "configPath": ["anomaly_detection_dashboards"],
  "requiredPlugins": ["navigation"],
  "optionalPlugins": [],
  "server": true,
  "ui": true
}

Using the above settings, I installed the plugin using:

./opensearch-dashboards-plugin install file://<path to build zip for the plugin>

The above command installs the plugin as:
customFolder

Issues Resolved

Part 3 and 4 of #322.

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

opensearch-ci-bot · 2021-06-04T23:39:59Z

✅ DCO Check Passed c23c30e

opensearch-ci-bot · 2021-06-04T23:55:58Z

❌ DCO Check Failed ffa60d1
Run ./dev-tools/signoff-check.sh remotes/origin/main ffa60d1cb32801517220cb8a52c6b79d9b91aecd to check locally
Use git commit with -s to add 'Signed-of-by: {EMAIL}' on impacted commits

opensearch-ci-bot · 2021-06-05T00:00:05Z

✅ DCO Check Passed 86111b9

opensearch-ci-bot · 2021-06-05T00:30:00Z

✅ DCO Check Passed 5fa4e2f

dblock

I think naming should be standardized. it looks like pluginFolder is an existing thing, soI would call the option pluginFolderas well, or targetPluginFolder.

dblock · 2021-06-07T15:05:06Z

src/cli_plugin/install/index.js

@@ -64,6 +64,7 @@ export function installCommand(program) {
      'length of time before failing; 0 for never fail',
      parseMilliseconds
    )
+    .option('-o, --override', 'override the folder name for plugin installation')


Should this match the folderName name? Aka --folderName= (can still be -o for the short version)

Updated the option to --folderName.

dblock · 2021-06-07T15:06:06Z

src/cli_plugin/install/install.js

@@ -59,11 +59,14 @@ export async function install(settings, logger) {

    del.sync(settings.tempArchiveFile, { force: true });

+    settings.plugins[0].pluginFolder = settings.override


We should be using the same name everywhere, if the option is folderName, then this would also be folderName and settings.folderName.

Made the naming consistent.

opensearch-ci-bot · 2021-06-07T18:39:59Z

❌ DCO Check Failed d45105d
Run ./dev-tools/signoff-check.sh remotes/origin/main d45105da3b1a00f53185ded958f0cd14563ee199 to check locally
Use git commit with -s to add 'Signed-of-by: {EMAIL}' on impacted commits

opensearch-ci-bot · 2021-06-07T18:55:59Z

✅ DCO Check Passed fb00d2f

dblock

I missed this in the first review, apologies. I think the value of folderName that's passed by the user should be used as-is, and not kebab-cased. It would be really confusing to users if I were to specify --folderName=myPlugin and end up with my-plugin, don't you think? Some test coverage of the above is needed in that case, too, then.

Thus, I think I'd want a property, settings.plugins[0].folderName, that defaults to kebabcase(id) on construction or some other place, and is overridden by -o. Move the calls to kebabcasing into whenever that value is assigned, so that we would reference it everywhere consistently. Also easier to test.

VachaShah · 2021-06-07T20:55:55Z

I missed this in the first review, apologies. I think the value of folderName that's passed by the user should be used as-is, and not kebab-cased. It would be really confusing to users if I were to specify --folderName=myPlugin and end up with my-plugin, don't you think? Some test coverage of the above is needed in that case, too, then.

The --folderName is a boolean flag to allow the cli to override the folder name, the folder name would be specified in the opensearch_dashboards.json file by the user just like the id, which would then be converted to kebab-case to follow the naming convention. The json file would look like this:

{
  "id": "anomalyDetectionDashboards",
  "folderName": "customFolder",
  "version": "1.0.0.0-beta1",
  "opensearchDashboardsVersion": "1.0.0-beta1",
  "configPath": ["anomaly_detection_dashboards"],
  "requiredPlugins": ["navigation"],
  "optionalPlugins": [],
  "server": true,
  "ui": true
}

I kept the conversion to kebab-case to keep the naming convention consistent. LMK what you think.

dblock · 2021-06-07T22:43:15Z

The --folderName is a boolean flag to allow the cli to override the folder name, the folder name would be specified in the opensearch_dashboards.json file by the user just like the id, which would then be converted to kebab-case to follow the naming convention. The json file would look like this:
{
  "id": "anomalyDetectionDashboards",
  "folderName": "customFolder",
  "version": "1.0.0.0-beta1",
  "opensearchDashboardsVersion": "1.0.0-beta1",
  "configPath": ["anomaly_detection_dashboards"],
  "requiredPlugins": ["navigation"],
  "optionalPlugins": [],
  "server": true,
  "ui": true
}
I kept the conversion to kebab-case to keep the naming convention consistent. LMK what you think.

Oh this is not at all how I understood this and is quite unusual! The developers of the plugin should be able to decide where the plugin is installed by default. This is how I believe this feature should be designed.

If I don't specify folderName in opensearch_dashboards.json then kebab_case(id) is used.
If I specify folderName in opensearch_dashboards.json, it's used "as is".
If I specify --folderName=X during installation, X is used as folder name.

That seems much more straightforward, doesn't it?

VachaShah · 2021-06-07T23:22:49Z

Oh this is not at all how I understood this and is quite unusual! The developers of the plugin should be able to decide where the plugin is installed by default. This is how I believe this feature should be designed.

The first two points are definitely straightforward. For the third one, I think if the user is allowed to enter the plugin folder name in the cli command itself, it will result in problems during the next install of the same plugin. When a plugin is installed, the logic first checks for an existing installation, this is checked using the plugin's id and the plugin's folderName both specified in the opensearch_dashboards.json. If the installation already exists, the user is given an error specifying the issue. If the folder name is given through cli using --folderName=X, there will be no way for the logic to check for existing installation next time and it will result in the same plugin being installed more than once. Do you think we should skip the point 3?

dblock · 2021-06-08T15:02:58Z

Oh this is not at all how I understood this and is quite unusual! The developers of the plugin should be able to decide where the plugin is installed by default. This is how I believe this feature should be designed.

The first two points are definitely straightforward. For the third one, I think if the user is allowed to enter the plugin folder name in the cli command itself, it will result in problems during the next install of the same plugin. When a plugin is installed, the logic first checks for an existing installation, this is checked using the plugin's id and the plugin's folderName both specified in the opensearch_dashboards.json. If the installation already exists, the user is given an error specifying the issue. If the folder name is given through cli using --folderName=X, there will be no way for the logic to check for existing installation next time and it will result in the same plugin being installed more than once. Do you think we should skip the point 3?

Makes sense to me, agreed.

opensearch-ci-bot · 2021-06-08T20:13:59Z

✅ DCO Check Passed 8a8327a

opensearch-ci-bot · 2021-06-11T22:38:31Z

✅ DCO Check Passed 58edde7

dblock

Please refactor and add tests for the refactored folder name function. This also needs tests for that the target directory is settings.folderName when specified, and is kebab-case of id when not.

dblock · 2021-06-14T14:25:13Z

src/cli_plugin/install/install.js

@@ -63,7 +63,10 @@ export async function install(settings, logger) {

    assertVersion(settings);

-    const targetDir = path.join(settings.pluginDir, kebabCase(settings.plugins[0].id));
+    const folderName = settings.plugins[0].folderName


Can this condition move into a targetFolderName() method in settings (might need to be extended into a class with methods), and use .targetFolderName() everywhere it's needed? This would avoid duplication, be more OOO, and would be easy to write unit tests against.

Moved this to getTargetFolderName() function.

dblock · 2021-06-14T14:26:00Z

src/cli_plugin/install/opensearch_dashboards.js

@@ -47,6 +47,18 @@ export function existingInstall(settings, logger) {
  } catch (e) {
    if (e.code !== 'ENOENT') throw e;
  }
+  if (settings.plugins[0].folderName !== '') {


The duplicate check for both .id and .folderName is telling me that there should only be one check here, instead of .id it would be using .targetFolderName().

Updated the logic to check for the targetFolderName.

dblock · 2021-06-14T14:28:39Z

src/cli_plugin/install/zip.js

@@ -82,6 +82,10 @@ export function analyzeArchive(archive) {
            plugins.push({
              id: manifest.id,
              stripPrefix: match[1],
+              folderName:


This should just be .manifest.folderName. There's nothing wrong with a null value, it shouldn't become blank if it's not set. If you move the logic to resolve targetFolderName into a function you can check for null or blank there and return id if the value of folderName is either.

Updated to move the logic to getTargetFolderName function.

opensearch-ci-bot · 2021-06-15T21:42:30Z

✅ DCO Check Passed 7b02a01

opensearch-ci-bot · 2021-06-15T22:22:40Z

✅ DCO Check Passed fcc3bdc

dblock

A lot cleaner! What's this removed code? Otherwise LGTM.

dblock · 2021-06-16T14:22:54Z

src/cli_plugin/install/opensearch_dashboards.js

@@ -38,10 +38,11 @@ import { versionSatisfies, cleanVersion } from '../../legacy/utils/version';

 export function existingInstall(settings, logger) {
  try {
-    statSync(path.join(settings.pluginDir, kebabCase(settings.plugins[0].id)));


You may have been right to worry that previous versions of plugins may have been installed into settings.plugins[0].id (1.1 installation on top of 1.0). Is that a valid concern. If so, make an array of all possible folder names, and do a foreach on them to check.

One of the cases I can think of now is the user first used the id of the plugin to install it, then added a folderName property to opensearch_dashboards.json and tried installing again. It should fail saying the plugin is already installed. I will add both the checks to be on the safer side.

dblock · 2021-06-16T14:25:30Z

src/cli_plugin/list/list.js

-        logger.log(pkg.id + '@' + pkg.version);
-      } catch (e) {
-        throw new Error('Unable to read opensearch_dashboards.json file for plugin ' + name);
-      }


What's the reason to remove this?

For the list command, the output printed was <pluginId>@<pluginVersion> but this same output could not be used in the remove command, this was mentioned in the same issue #322 in the latest comments, I removed this so that the actual folder name can be printed which the user can use to remove the plugin with the remove command. LMK if you think this could be better.

opensearch-ci-bot · 2021-06-16T19:04:29Z

✅ DCO Check Passed 72cc7e6

Signed-off-by: Vacha Shah <[email protected]>

opensearch-ci-bot · 2021-06-16T20:16:27Z

✅ DCO Check Passed a2ff1fd

dblock

Looks good. Maybe someone from Dashboards wants to review this as well, since I'm not super familiar with the codebase - @mihirsoni maybe?

VachaShah · 2021-09-08T21:09:40Z

Going to close this PR, can be picked up when required.

VachaShah force-pushed the allow_custom_folder_name_for_plugins branch from ffa60d1 to 86111b9 Compare June 4, 2021 23:57

VachaShah force-pushed the allow_custom_folder_name_for_plugins branch from 86111b9 to 5fa4e2f Compare June 5, 2021 00:28

dblock requested changes Jun 7, 2021

View reviewed changes

VachaShah mentioned this pull request Jun 7, 2021

Proposal: Allow custom plugin path when installing #322

Open

VachaShah force-pushed the allow_custom_folder_name_for_plugins branch from d45105d to fb00d2f Compare June 7, 2021 18:54

VachaShah requested a review from dblock June 7, 2021 19:11

dblock requested changes Jun 7, 2021

View reviewed changes

VachaShah force-pushed the allow_custom_folder_name_for_plugins branch from fb00d2f to 8a8327a Compare June 8, 2021 20:11

VachaShah requested a review from dblock June 8, 2021 21:28

VachaShah force-pushed the allow_custom_folder_name_for_plugins branch from 8a8327a to 58edde7 Compare June 11, 2021 22:35

dblock requested changes Jun 14, 2021

View reviewed changes

VachaShah force-pushed the allow_custom_folder_name_for_plugins branch from 58edde7 to 7b02a01 Compare June 15, 2021 21:40

VachaShah force-pushed the allow_custom_folder_name_for_plugins branch from 7b02a01 to fcc3bdc Compare June 15, 2021 22:17

VachaShah requested a review from dblock June 16, 2021 04:59

dblock requested changes Jun 16, 2021

View reviewed changes

VachaShah force-pushed the allow_custom_folder_name_for_plugins branch from fcc3bdc to 72cc7e6 Compare June 16, 2021 19:01

Allowing custom folder name for plugin installation

a2ff1fd

Signed-off-by: Vacha Shah <[email protected]>

VachaShah force-pushed the allow_custom_folder_name_for_plugins branch from 72cc7e6 to a2ff1fd Compare June 16, 2021 20:13

VachaShah requested a review from dblock June 16, 2021 21:21

dblock approved these changes Jun 17, 2021

View reviewed changes

VachaShah closed this Sep 8, 2021

		@@ -59,11 +59,14 @@ export async function install(settings, logger) {

		del.sync(settings.tempArchiveFile, { force: true });

		settings.plugins[0].pluginFolder = settings.override

Allowing custom folder name for plugin installation #446

Allowing custom folder name for plugin installation #446

Conversation

VachaShah commented Jun 4, 2021 • edited Loading

Description

Testing

Issues Resolved

Check List

opensearch-ci-bot commented Jun 4, 2021

opensearch-ci-bot commented Jun 4, 2021

opensearch-ci-bot commented Jun 5, 2021

opensearch-ci-bot commented Jun 5, 2021

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-ci-bot commented Jun 7, 2021

opensearch-ci-bot commented Jun 7, 2021

dblock left a comment • edited Loading

Choose a reason for hiding this comment

VachaShah commented Jun 7, 2021

dblock commented Jun 7, 2021

VachaShah commented Jun 7, 2021

dblock commented Jun 8, 2021

opensearch-ci-bot commented Jun 8, 2021

opensearch-ci-bot commented Jun 11, 2021

dblock left a comment

Choose a reason for hiding this comment

dblock Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VachaShah Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

dblock Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-ci-bot commented Jun 15, 2021

opensearch-ci-bot commented Jun 15, 2021

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VachaShah Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

opensearch-ci-bot commented Jun 16, 2021

opensearch-ci-bot commented Jun 16, 2021

dblock left a comment

Choose a reason for hiding this comment

VachaShah commented Sep 8, 2021

VachaShah commented Jun 4, 2021 •

edited

Loading

dblock left a comment •

edited

Loading

dblock Jun 14, 2021 •

edited

Loading

VachaShah Jun 14, 2021 •

edited

Loading

dblock Jun 14, 2021 •

edited

Loading

VachaShah Jun 16, 2021 •

edited

Loading