Default to converting folder name for cli plugin to kebab-case #357

VachaShah · 2021-05-14T00:26:54Z

Description

This change intends to default the folder name for plugins being installed through cli to kebab-case.

To test the change, I ran ./bin/opensearch-dashboards-plugin install file:///anomalyDetectionDashboards-1.0.0.0-beta1.zip to verify the folder name generated which is now in kebab-case: anomaly-detection-dashboards.

Issues Resolved

Part 1 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-05-14T00:40:56Z

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

opensearch-ci-bot · 2021-05-14T00:46:32Z

✅ DCO Check Passed 12a803d

ohltyler

LGTM, thanks for the change!

dblock · 2021-05-19T13:49:07Z

We should have a test for this, too. But don't let that hold the merge.

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

opensearch-ci-bot · 2021-05-19T23:04:52Z

✅ DCO Check Passed 917c078

VachaShah · 2021-05-19T23:39:34Z

We should have a test for this, too. But don't let that hold the merge.

Sure. I have also made a change when checking the existing installation for a plugin which is a pre-check to installing the plugin. I think the kebab-case would have to added there as well.

mihirsoni · 2021-05-20T04:37:58Z

src/cli_plugin/install/install.js

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

    assertVersion(settings);

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


@VachaShah does this have any co-relation with removing plugin as well ? https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/cli_plugin/remove/remove.js

@VachaShah does this have any co-relation with removing plugin as well ? https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/cli_plugin/remove/remove.js

This would not affect the removing plugin logic as it does not take into consideration the plugin id.

dblock · 2021-05-20T15:08:12Z

This can be done later, but now that we are duplicating logic I would introduce settings.plugins[0].path that would default to kebabCase(id). Then it can be set as well and anywhere we assume ID = path (now at least in 2 places) can refer to it.

VachaShah · 2021-05-20T17:12:44Z

This can be done later, but now that we are duplicating logic I would introduce settings.plugins[0].path that would default to kebabCase(id). Then it can be set as well and anywhere we assume ID = path (now at least in 2 places) can refer to it.

Sure, I can do this as part of the second item on the checklist.

mihirsoni

LGTM ! Thanks @VachaShah for the fix.

kavilla

LGTM!

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

opensearch-project#357)" This reverts commit 747ef8e. Reverting for now because the full impact is not known and requires subsequent commits to mitigate confusion related to CLI output. Also, it seems like in the code there exists verification on the code that plugins should explicitly be camelCase. So this merits more discussion. Issues related: opensearch-project#322 opensearch-project#465 opensearch-project#366 Signed-off-by: Kawika Avilla <[email protected]>

#357)" (#578) This reverts commit 747ef8e. Reverting for now because the full impact is not known and requires subsequent commits to mitigate confusion related to CLI output. Also, it seems like in the code there exists verification on the code that plugins should explicitly be camelCase. So this merits more discussion. Issues related: #322 #465 #366 Signed-off-by: Kawika Avilla <[email protected]>

opensearch-project#357)" (opensearch-project#578) This reverts commit 747ef8e. Reverting for now because the full impact is not known and requires subsequent commits to mitigate confusion related to CLI output. Also, it seems like in the code there exists verification on the code that plugins should explicitly be camelCase. So this merits more discussion. Issues related: opensearch-project#322 opensearch-project#465 opensearch-project#366 Signed-off-by: Kawika Avilla <[email protected]>

#357)" (#583) This reverts commit 747ef8e. Reverting for now because the full impact is not known and requires subsequent commits to mitigate confusion related to CLI output. Also, it seems like in the code there exists verification on the code that plugins should explicitly be camelCase. So this merits more discussion. Issues related: #322 #465 #366 Signed-off-by: Kawika Avilla <[email protected]>

#357)" (#584) This reverts commit 747ef8e. Reverting for now because the full impact is not known and requires subsequent commits to mitigate confusion related to CLI output. Also, it seems like in the code there exists verification on the code that plugins should explicitly be camelCase. So this merits more discussion. Issues related: #322 #465 #366 Signed-off-by: Kawika Avilla <[email protected]>

…ebab-case (opensearch-project#357)" (opensearch-project#578)" This reverts commit 45df886.

VachaShah force-pushed the update_cli_plugin_folder_name_case branch from 41460d3 to 12a803d Compare May 14, 2021 00:44

ohltyler previously approved these changes May 18, 2021

View reviewed changes

dblock previously approved these changes May 18, 2021

View reviewed changes

Default to converting folder name for cli plugin to kebab-case

917c078

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

VachaShah dismissed stale reviews from dblock and ohltyler via 917c078 May 19, 2021 22:37

VachaShah force-pushed the update_cli_plugin_folder_name_case branch from 12a803d to 917c078 Compare May 19, 2021 22:37

mihirsoni reviewed May 20, 2021

View reviewed changes

mihirsoni approved these changes May 20, 2021

View reviewed changes

dblock mentioned this pull request May 23, 2021

Plugin artifacts should produce kebab-cased filenames that match plugin directory #366

Open

kavilla self-requested a review May 24, 2021 15:57

kavilla approved these changes May 24, 2021

View reviewed changes

kavilla merged commit 747ef8e into opensearch-project:main May 25, 2021

VachaShah mentioned this pull request May 25, 2021

Proposal: Allow custom plugin path when installing #322

Open

VachaShah deleted the update_cli_plugin_folder_name_case branch May 25, 2021 18:45

kavilla pushed a commit that referenced this pull request May 25, 2021

Default to converting folder name for cli plugin to kebab-case (#357)

09bca29

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

ohltyler mentioned this pull request Jun 1, 2021

Run CI against 1.0.0-rc1 docker images opensearch-project/anomaly-detection-dashboards-plugin#36

Merged

1 task

kavilla mentioned this pull request Jun 11, 2021

[BUG]cannot remove securityDashboards in opensearch-dashboards:1.0.0-rc1 #465

Closed

kavilla added a commit to kavilla/OpenSearch-Dashboards-1 that referenced this pull request Jul 28, 2021

Revert "Revert "Default to converting folder name for cli plugin to k…

5469a87

…ebab-case (opensearch-project#357)" (opensearch-project#578)" This reverts commit 45df886.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default to converting folder name for cli plugin to kebab-case #357

Default to converting folder name for cli plugin to kebab-case #357

VachaShah commented May 14, 2021 •

edited

Loading

opensearch-ci-bot commented May 14, 2021

opensearch-ci-bot commented May 14, 2021

ohltyler left a comment

dblock commented May 19, 2021

opensearch-ci-bot commented May 19, 2021

VachaShah commented May 19, 2021

mihirsoni May 20, 2021

VachaShah May 20, 2021

dblock commented May 20, 2021

VachaShah commented May 20, 2021

mihirsoni left a comment

kavilla left a comment

Default to converting folder name for cli plugin to kebab-case #357

Default to converting folder name for cli plugin to kebab-case #357

Conversation

VachaShah commented May 14, 2021 • edited Loading

Description

Issues Resolved

Check List

opensearch-ci-bot commented May 14, 2021

opensearch-ci-bot commented May 14, 2021

ohltyler left a comment

Choose a reason for hiding this comment

dblock commented May 19, 2021

opensearch-ci-bot commented May 19, 2021

VachaShah commented May 19, 2021

mihirsoni May 20, 2021

Choose a reason for hiding this comment

VachaShah May 20, 2021

Choose a reason for hiding this comment

dblock commented May 20, 2021

VachaShah commented May 20, 2021

mihirsoni left a comment

Choose a reason for hiding this comment

kavilla left a comment

Choose a reason for hiding this comment

VachaShah commented May 14, 2021 •

edited

Loading