Proposal: Allow custom plugin path when installing

[ohltyler]

Describe the solution you'd like

Currently, when installing a plugin to dashboards via ./opensearch-dashboards-plugin-install file://<zip>, the plugin folder name is automatically created using the plugin id here, which is currently required to be camel case. This will create plugin folders looking like OpenSearch-Dashboards/plugins/<camelCasePluginId>. A camel case folder name doesn't follow standard naming conventions within Dashboards. For example, preinstalled plugins are in snake case (see here).

I propose to either change the default created directory for installed plugins to follow the conventions of snake case, or add a field in opensearch_dashboards.json config files for plugins to allow setting a custom plugin path.

[dblock]

I think we should do four things here, in order of priority:

1. Default to converting plugin ID to snake case for the folder name.
2. Allow plugin authors to declare the folder name for a (dashboards) plugin in the configuration.
3. Allow opensearch-dashboards-plugin-install to override the folder name.
4. Relocate pre-installed plugins to match this scheme.

[VachaShah]

I can take this.

[dblock]

This is what we are shipping in Beta 1.

testbox /tmp/opensearch-dashboards$ ls -la plugins/

alertingDashboards
anomalyDetectionDashboards
ganttChartDashboards
indexManagementDashboards
notebooksDashboards
queryWorkbenchDashboards
reportsDashboards
securityDashboards
traceAnalyticsDashboards

to match OpenSearch plugin naming convention these should become

dashboards-alerting
dashboards-anomaly-detection
dashboards-gantt-chart
dashboards-index-management
dashboards-notebooks
dashboards-query-workbench
dashboards-reports
dashboards-security
dashboards-trace-analytics

[ohltyler]

@dblock typical notation in dashboards has directories be in snake case, including the preinstalled plugins. Is it more important to be consistent with the OpenSearch plugins or existing Dashboards plugins / ecosystem?

[dblock]

@dblock typical notation in dashboards has directories be in snake case, including the preinstalled plugins. Is it more important to be consistent with the OpenSearch plugins or existing Dashboards plugins / ecosystem?

This is what I'm thinking:

1. Users don't see directories, so this is just for consistency, and developers
2. camelCase folders are potentially error prone, because some systems may be case-sensitive and others may not
3. camelCase folders are embarrassing
4. As a user, I'm least surprised when the folder name matches the plugin name, which should match the distribution .zip/tar.gz.
5. Consistency between OpenSearch plugins and OpenSearch dashboard plugins isn't necessary, but I think (4) is.
6. Since we're trying to fix camelCase, maybe it's a good opportunity to do (4).
7. I am OCD, sosnake-case vs. snake_case bothers me, but only me :)

WDYT @ohltyler ?

[ohltyler]

@dblock Definitely agree about not using camel case. I guess my question was more about should we try to be consistent with the OpenSearch plugins (using kebab-case) vs. consistency with the existing Dashboards plugins / ecosystem (using snake_case).

[dblock]

Thanks for reminding me it's called kebab-case! Do you have a strong opinion? We should go with yours until someone else has a stronger one.

[ohltyler]

I think kebab-case may be preferred here, so all OpenSearch and Dashboards plugins can follow unified naming conventions regarding artifacts/zips and folder names. Although the installed folders will follow different conventions than the preinstalled plugins which are in snake_case, I feel this has less impact and is more of a hidden implementation detail.

[dblock]

Let's relocate the preinstalled plugins too then? cc: @VachaShah

[ohltyler]

@dblock to limit the blast radius, should we limit the changes to only default the new plugins installed via cli to be kebab-case? By renaming the pre-installed plugins in the src/plugins dir, it will break consistency within the core Dashboards repo itself (going from snake_case to kebab-case, and may have some other implications if folks have workflows or scripts relying on those filepaths, etc.

[dblock]

Those paths aren't APIs, so I would say we can make that change, better now than later, but I also don't feel strongly about it.

[VachaShah]

I discussed with @nknize on this, he mentioned that if there is no directory name convention enforcement code, then we would be fine to rename the pre-installed ones and not break anything. A little side track to that I also found that the folder name for the same plugin is different when installed through cli vs copied to plugins/ and built with dashboards. For example: I tried two ways of installing the plugin anomaly-detection-dashboards, one with copying the plugin in plugins/ and then running yarn build on the dashboards repo which generated the plugin in the build with name anomaly-detection-dashboards-plugin, then I installed the same plugin through cli which created the plugin with name anomalyDetectionDashboards (this is the original code without kebab-case, with kebab-case it is anomaly-detection-dashboards) in the build directory, so the same plugin is twice:

~/OpenSearch-Dashboards/build/opensearch-dashboards-1.0.0-SNAPSHOT-linux-x64/plugins$ ls
anomaly-detection-dashboards-plugin  anomalyDetectionDashboards

Now the cli install has a check for existing install, so the second time I try to install the same plugin through cli, it recognizes it and does not allow the installation to go through.

So there are a few things here:

1. Why is the behavior (folder name) of the plugin installation different between the copied plugin and the cli install path. Is this an expected behavior? I think this would be better answered by the dashboards team.
2. Which I feel coincides with 1. that @nknize pointed out to add some enforcement code for the directory name to manage it across various install methods.
3. To do the same behavior for pre-installed plugins.

@dblock Please let me know if you have any preferences here.

[VachaShah]

Also, for the pre-installed plugins, it looks like its not just the folder names but also sub-folders and files all in snake_case. For example for plugin advanced_settings, there are sub-folders under it: https://github.com/opensearch-project/OpenSearch-Dashboards/tree/main/src/plugins/advanced_settings/public with snake_case naming conventions and files within them with the same: https://github.com/opensearch-project/OpenSearch-Dashboards/tree/main/src/plugins/advanced_settings/public/management_app.

Do we want to change all of this to kebab-case?

[dblock]

1. Why is the behavior (folder name) of the plugin installation different between the copied plugin and the cli install path. Is this an expected behavior? I think this would be better answered by the dashboards team.

It's clearly not a logical or desired behavior, I would open bugs (and fix them). I'd expect that installing a plugin via whatever way produces the same result.

2. Which I feel coincides with 1. that @nknize pointed out to add some enforcement code for the directory name to manage it across various install methods.

Agreed.

3. To do the same behavior for pre-installed plugins.

Agreed.

@dblock Please let me know if you have any preferences here.

I think you now have identified a whole bunch of problems and I am looking forward to fixes!

[dblock]

Do we want to change all of this to kebab-case?

I think that the answer is yes because it will make it all consistent. We should also document this in the docs, and in https://github.com/opensearch-project/opensearch-plugins/blob/main/CONVENTIONS.md once we're done.

[VachaShah]

Thank you @dblock. I am going to separate out the problems into various issues so they are easier to manage.

For this issue, the following needs to be addressed:

1. Default to convert the folder name for a cli-installed plugin to kebab-case. (Addressed in PR: Default to converting folder name for cli plugin to kebab-case #357)
2. Convert the pre-installed plugins folder name to kebab-case. (Addressed in the following PRs Renaming advanced_settings plugin folder to kebab-case #394 Renaming apm_oss plugin folder to kebab-case #395 Renaming bfetch, charts, console plugin folder to kebab-case #396 Renaming dashboard, data plugin folder to kebab-case #397 Renaming dev_tools, discover plugin folder to kebab-case #398 Renaming embeddable, expressions plugin folder to kebab-case #399 Renaming home plugin folder to kebab-case #400 Renaming index_pattern_management plugin folder to kebab-case #401 Renaming input_control_vis, legacy_export plugin folder to kebab-case #402 Renaming management, maps_legacy plugin folder to kebab-case #403 Renaming navigation, opensearch_dashboards_legacy plugin folder to kebab-case #404 Renaming opensearch_dashboards_overview, opensearch_dashboards_react … #407 Renaming osd_usage_collection, osd_ui_shared plugin folder to kebab-case #408 Renaming opensearch_dashboards_utils plugin folder to kebab-case #409 Renaming region_map, saved_objects plugin folder to kebab-case #410 Renaming saved_objects_management, share plugin folder to kebab-case #411 Renaming telemetry related plugins folder to kebab-case #412 Renaming tile_map, timeline plugin folder to kebab-case #414 Renaming ui_actions, url_forwarding plugin folder to kebab-case #415 Renaming vis_type_metric, vis_type_table plugin folder to kebab-case #418 Renaming vis_default_editor, vis_type_markdown plugin folder to kebab… #419 Renaming usage_collection plugin folder to kebab-case #420 Renaming vis_type_vislib plugin folder to kebab-case #421 Renaming vis_type_vega, vis_type_xy plugin folder to kebab-case #422 Renaming vis_type_timeseries plugin folder to kebab-case #423 Renaming visualizations plugin folder to kebab-case #424 Renaming vis_type_tagcloud, vis_type_timeline plugin folder to kebab-… #425)
3. Allow plugin authors to declare the folder name for a (dashboards) plugin in the configuration. (Addressed in PR: Allowing custom folder name for plugin installation #446)
4. Allow opensearch-dashboards-plugin install to override the folder name. (Addressed in PR: Allowing custom folder name for plugin installation #446)

[mihirsoni]

@dblock I think we should just make changes for external plugin for now and not change any internal ones. We should do that gradually as part of 2.0 as it doesn't have user facing requirement after understanding their code bas and reasons for it.

[dblock]

@mihirsoni Don't feel strongly about it, your call!

[mihirsoni]

If we can do it gradually with smaller PRs, to ensure no git history is lost as well functional tests passes on each one should be good, my only point s we can keep making progress, I remember kibana had earlier using - but changed to camelCase as part of new framework need to understand the reason why it is like that.

Overall my point was, not to add any scope for GA, we can do it as part of 1.1 and keep making progress. Just my thoughts if we have extra bandwidth to take care we'll take in.

[ahopp]

@tmarkley What's the plan with this? Do we have next steps?

[kavilla]

To provide further insight. When generating plugins, the plugin does come out as the format as snake case.

node scripts/generate_plugin
? Plugin name (use camelCase): i18nPlugin

produces: i18n_plugin.

During the build process

cd plugins/i18n_plugin
yarn plugin-helpers build

Will produce a zip based on the ID of the plugin because at this point the folder name of the plugin can be anything it can be camelCase. So the build process relies on producing a camelCase zip of the plugin. i18nPlugin.zip.

When installing, the build process doesn't care about the name of the file just as long as it can find it. It unzips it and then targets a folder that uses the ID. If we would like we can modify the following line to convert the camelCase to snake_case:

https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/cli_plugin/install/install.js#L65

So that when it produces the plugin it can follow the format as core plugins. That or build in the functionality that lets the plugin folder name be specified as dB mentioned.

My opinion is to not prioritize the specifying the zip name when running yarn plugin-helpers build. The output can be easily renamed with one command as opposed to adding a new argument. We can create a good first issue for that.

As far as the kebab vs snake_case, I don't think it's worth the amount of risk anymore.

[joshuarrrr]

@kavilla This is over a year old - can we close?