Allowing custom folder name for plugin installation

[VachaShah]

Signed-off-by: Vacha Shah vachshah@amazon.com

Description

This PR allows the user to add a custom folder name for plugin cli installation. This custom folder name can be specified in the plugin properties file for the plugin, when this property is specified, the plugin would be installed with the custom folder name otherwise it will be installed with the name of the plugin. This feature would be available for versions after 1.0.0.

The feature includes:

1. Support for custom folder name for plugin installation after 1.0.0 versions with backwards compatibility.
2. The install plugin command will display the folder name where the plugin is being installed.
3. The list plugin command will continue to display the plugin name to maintain backwards compatibility.
4. The remove plugin command will continue to take in the plugin name to remove it even though the plugin is installed in a custom folder to maintain backwards compatibility.
5. During installation or removal, when checking if a plugin already exists, any folder previously installed with the plugin name or custom name would be determined.

Testing

• For manual testing, I added the customFolderName property in build.gradle for job-scheduler plugin as follows:

  opensearchplugin {    
  name 'opensearch-job-scheduler'    
  description 'OpenSearch Job Scheduler plugin'    
  classname 'org.opensearch.jobscheduler.JobSchedulerPlugin'
  customFolderName 'custom-folder'
  }

  1. When installing the plugin with the above properties using ./opensearch-plugin install <path to build file>, this resulted in the plugin being installed in folder: custom-folder with an output message as follows:

     -> Installed opensearch-job-scheduler with folder name custom-folder

  2. When the plugins are being listed using ./opensearch-plugin list, this resulted in the output: opensearch-job-scheduler.
  3. To remove the plugin, I used ./opensearch-plugin remove opensearch-job-scheduler.

Issues Resolved

686

Check List

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

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[opensearch-ci-bot]

:white_check_mark:   Gradle Wrapper Validation success 838229ded3c80bc2764a66935638cf71e7cfbf1b

[opensearch-ci-bot]

:white_check_mark:   DCO Check Passed 838229ded3c80bc2764a66935638cf71e7cfbf1b

[opensearch-ci-bot]

:white_check_mark:   Gradle Precommit success 838229ded3c80bc2764a66935638cf71e7cfbf1b

[opensearch-ci-bot]

:white_check_mark:   DCO Check Passed fc944e6d78281cd183c91826abd8467f00c728aa

[opensearch-ci-bot]

:white_check_mark:   Gradle Wrapper Validation success fc944e6d78281cd183c91826abd8467f00c728aa

[opensearch-ci-bot]

:white_check_mark:   Gradle Precommit success fc944e6d78281cd183c91826abd8467f00c728aa

[dblock]

We'll need a corresponding doc change I believe for 1.1, cc: @aetter, maybe point @VachaShah where to make it?

[opensearch-ci-bot]

:white_check_mark:   Gradle Wrapper Validation success 3f256cbc83359f6f46866102859ecfc775f33b27

[opensearch-ci-bot]

:white_check_mark:   DCO Check Passed 3f256cbc83359f6f46866102859ecfc775f33b27

[opensearch-ci-bot]

:white_check_mark:   Gradle Precommit success 3f256cbc83359f6f46866102859ecfc775f33b27

[dblock]

start gradle check

[opensearch-ci-bot]

:x:   Gradle Check failure 3f256cbc83359f6f46866102859ecfc775f33b27 Log 242

Reports 242

[dblock]

@VachaShah not sure what's up with CI failures ^

[VachaShah]

@VachaShah not sure what's up with CI failures ^

I will look into these.

[adnapibar]

I don't think changing the behavior of how opensearch-plugin command works is a good idea. The list subcommand should list the name of the plugins and not the folder name. Similarly to remove a plugin, the remove subcommand should take the name of the plugin and not a folder name.

[VachaShah]

I don't think changing the behavior of how opensearch-plugin command works is a good idea. The list subcommand should list the name of the plugins and not the folder name. Similarly to remove a plugin, the remove subcommand should take the name of the plugin and not a folder name.

We are not changing the behavior of the list or remove commands. They still work the same way as before, I added the behavior in the description just to show the behavior. This is just to provide the user a way to customize the folder name during installation.

[adnapibar]

I don't think changing the behavior of how opensearch-plugin command works is a good idea. The list subcommand should list the name of the plugins and not the folder name. Similarly to remove a plugin, the remove subcommand should take the name of the plugin and not a folder name.

We are not changing the behavior of the list or remove commands. They still work the same way as before, I added the behavior in the description just to show the behavior. This is just to provide the user a way to customize the folder name during installation.

Got it - Just to confirm, if I install a plugin with a custom folder name, I can still remove it using the plugin name? And when I run 'opensearch-pugin list` it still lists the original plugin name?

[VachaShah]

I don't think changing the behavior of how opensearch-plugin command works is a good idea. The list subcommand should list the name of the plugins and not the folder name. Similarly to remove a plugin, the remove subcommand should take the name of the plugin and not a folder name.

We are not changing the behavior of the list or remove commands. They still work the same way as before, I added the behavior in the description just to show the behavior. This is just to provide the user a way to customize the folder name during installation.

Got it - Just to confirm, if I install a plugin with a custom folder name, I can still remove it using the plugin name? And when I run 'opensearch-pugin list` it still lists the original plugin name?

No the list command lists the folder name and the remove command uses the folder name to remove the plugin.

[adnapibar]

No the list command lists the folder name and the remove command uses the folder name to remove the plugin.

Okay, that's what I was referring to as a change in behavior. I think, we need to make this change in a way that keeps the old behavior as it is and doesn't break it.

[dblock]

No the list command lists the folder name and the remove command uses the folder name to remove the plugin.

Okay, that's what I was referring to as a change in behavior. I think, we need to make this change in a way that keeps the old behavior as it is and doesn't break it.

So we think users today think of adding/removing as using the ID not the folder. They just happen to be the same and the code uses folder. Sounds like you should be able to specify either. Rabi is right.

[VachaShah]

Currently for this feature, the things that have been done are:

• Allow the user to add a property to plugin-descriptor.properties for custom folder name.
• The Install command checks if the target folder name exists which is id of the plugin if no custom folder name is specified. It allows the user to install the plugin with a custom folder name if no such folder exists. Also adding the individual check for both the name of the plugin and the folder name specified to see if the plugin exists either way.
• Adding a scoping for the version for this new property.

Other cases that would be required to maintain the backwards compatibility as mentioned by @adnapibar:

• The list command should be backwards compatible i.e. it should still list the name of the plugin even if the plugin is installed using custom folder name.
• The remove command should be backwards compatible i.e. it should still take the name of the plugin as the input even though the plugin to be removed is installed in a custom folder. For this, we might have to scan each plugin folder in order to match the plugin name with the one being passed in the command to determine which plugin needs to be removed since we do not have the custom folder name information in the command.
• There can be a case where the same plugin is installed with different folder names which will break OpenSearch, to prevent this, we would have to scan each plugin folder to match the name of the plugin being installed to prevent it from being installed multiple times.
• There might also be some other manual use cases other than above which would have to be tested to make sure this feature works as expected.

@adnapibar @dblock Any thoughts?

[dblock]

Other cases that would be required to maintain the backwards compatibility as mentioned by @adnapibar:

Generally I think we need to fix everything that is broken by the change, so the entire list below.

• The list command should be backwards compatible i.e. it should still list the name of the plugin even if the plugin is installed using custom folder name.

Maybe we can add showing the plugin ID, name and folder to avoid confusion? I am not personally familiar with how this command works.

• The remove command should be backwards compatible i.e. it should still take the name of the plugin as the input even though the plugin to be removed is installed in a custom folder. For this, we might have to scan each plugin folder in order to match the plugin name with the one being passed in the command to determine which plugin needs to be removed since we do not have the custom folder name information in the command.

Agreed.

• There can be a case where the same plugin is installed with different folder names which will break OpenSearch, to prevent this, we would have to scan each plugin folder to match the name of the plugin being installed to prevent it from being installed multiple times.

I think when removing a plugin scanning the list of all known folder names is a good idea.

[opensearch-ci-bot]

:white_check_mark:   Gradle Wrapper Validation success 82f560f74a660e017ea19ada4e65ade83402bbc0

[opensearch-ci-bot]

:white_check_mark:   DCO Check Passed 82f560f74a660e017ea19ada4e65ade83402bbc0

[opensearch-ci-bot]

:white_check_mark:   Gradle Precommit success 82f560f74a660e017ea19ada4e65ade83402bbc0

[adnapibar]

start gradle check

[opensearch-ci-bot]

:x:   Gradle Check failure 82f560f74a660e017ea19ada4e65ade83402bbc0 Log 263

Reports 263

[opensearch-ci-bot]

:white_check_mark:   Gradle Wrapper Validation success 82f560f74a660e017ea19ada4e65ade83402bbc0

[opensearch-ci-bot]

:white_check_mark:   DCO Check Passed 82f560f74a660e017ea19ada4e65ade83402bbc0

[opensearch-ci-bot]

:white_check_mark:   Gradle Precommit success 82f560f74a660e017ea19ada4e65ade83402bbc0

[dblock]

@VachaShah looks like there are test failures?

[VachaShah]

@VachaShah looks like there are test failures?

Yeah I fixed the original ones, these are from a recent merge from main for the version change, I will fix all of them together once I have the List and Remove command features updated as well.

[opensearch-ci-bot]

:white_check_mark:   Gradle Wrapper Validation success cf5c0c55c95660785c9aa70fc8016fb0355e458b

[opensearch-ci-bot]

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

[opensearch-ci-bot]

:white_check_mark:   Gradle Precommit success cf5c0c55c95660785c9aa70fc8016fb0355e458b

[opensearch-ci-bot]

:white_check_mark:   Gradle Wrapper Validation success 0d93b7faab4eb3a53ac64df1651cfa285ac3817f

[opensearch-ci-bot]

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

[opensearch-ci-bot]

:white_check_mark:   Gradle Precommit success 0d93b7faab4eb3a53ac64df1651cfa285ac3817f

[opensearch-ci-bot]

:x:   Gradle Wrapper Validation failure d4b67eab7ee4cffb7ffea42bb59654c46f610667

:alert: Gradle Wrapper integrity has been altered

[opensearch-ci-bot]

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

[opensearch-ci-bot]

:white_check_mark:   DCO Check Passed 7ae84050b32e1c8df1f61e29a844874a661cd1fb

[opensearch-ci-bot]

:x:   Gradle Precommit failure d4b67eab7ee4cffb7ffea42bb59654c46f610667 Log 792

[opensearch-ci-bot]

:white_check_mark:   Gradle Wrapper Validation success 7ae84050b32e1c8df1f61e29a844874a661cd1fb

[opensearch-ci-bot]

:white_check_mark:   Gradle Precommit success 7ae84050b32e1c8df1f61e29a844874a661cd1fb

[opensearch-ci-bot]

:white_check_mark:   Gradle Wrapper Validation success 45f9a5f8eb758f05d7d68d3fb0e949058800bf55

[opensearch-ci-bot]

:white_check_mark:   DCO Check Passed 45f9a5f8eb758f05d7d68d3fb0e949058800bf55

[opensearch-ci-bot]

:white_check_mark:   Gradle Precommit success 45f9a5f8eb758f05d7d68d3fb0e949058800bf55

[opensearch-ci-bot]

:white_check_mark:   Gradle Wrapper Validation success c77a550d57b5a2187d784839b8b7fc701ff16599

[opensearch-ci-bot]

:white_check_mark:   DCO Check Passed c77a550d57b5a2187d784839b8b7fc701ff16599

[opensearch-ci-bot]

:white_check_mark:   Gradle Precommit success c77a550d57b5a2187d784839b8b7fc701ff16599