opensearch-project / opensearch-plugins

For all things OpenSearch plugins. You want to install, or develop a plugin? You've come to the right place.
Apache License 2.0
51 stars 61 forks source link

Standardize file locations for supporting files #127

Closed sean-zheng-amazon closed 2 years ago

sean-zheng-amazon commented 2 years ago

(This is a meta issue for tracking implementation of standard supporting file location. Summarized from proposal: https://github.com/opensearch-project/opensearch-plugins/issues/91)

Is your feature request related to a problem? Most of OpenSearch plugins contain supporting files like config files or libraries. These files are currently stored in different locations with different convention in plugins. This inconsistency could cause confusing for cluster administrators since they need to figure out these conventions plugin by plugin.

What solution would you like? We should standardize the file location across plugins by implementing following conventions:

What alternatives have you considered? See discussion in proposal: https://github.com/opensearch-project/opensearch-plugins/issues/91

anirudha commented 2 years ago

can we do a plugin creator like : https://start.spring.io/ to help new plugins get started ( dashboards and opensearch ) this also aligns a standard for folder setup

jmazanec15 commented 2 years ago

I believe that "config" and "bin" convention already exists: https://github.com/opensearch-project/OpenSearch/blob/1.3.0/distribution/tools/plugin-cli/src/main/java/org/opensearch/plugins/InstallPluginCommand.java#L128-L135.

I am wondering if a better way of going about this would be for the plugin cli to enforce these conventions (as it does for bin and config)?

peternied commented 2 years ago

@sean-zheng-amazon we have been looking into this from the security plugin, it is going to break a large amount of our documentation / related to change the tooling paths that we have used for the audit/hash/securityadmin scripts.

This does not seem high value for the breaking change. Rather than centralize these scripts, what about creating an OpenSearch Admin tool that abstracts these paths entirely? FYI @setiah that has been looking into this concept.

sean-zheng-amazon commented 2 years ago

@peternied Just to clarify, this is not about centralizing scripts/supporting files, they will still be located at plugin folders, just with the same naming convention to reduce ux confuse. That being said, we are open to other solutions if there is a better way. Can you tell a bit more details on the admin tool?

peternied commented 2 years ago

Here is some of the thoughts that @setiah has captured https://github.com/opensearch-project/opensearch-build/issues/1649#issuecomment-1091212121

The request to change from tools -> bin this impacts standard security plugin setup processes that are used for configuration, our customer base will be severely impacted by this change. These tools are not in an ideal state today that have little to no error handling that would making detecting and resolving this issues difficult.

peternied commented 2 years ago

An example of such a tool is the securityadmin.sh, see its documentation https://opensearch.org/docs/latest/security-plugin/configuration/security-admin/.

It currently resides at /opensearch/plugins/opensearch-security/tools/securityadmin.sh and under this proposal it would move to /opensearch/plugins/opensearch-security/bin/securityadmin.sh this would break scripts that depend on this location in the file system

sean-zheng-amazon commented 2 years ago

We are of course expecting breaking changes like this with the directory name change, that's why we want to do this with the major version bump. We can update tech docs to mitigate the risk. More importantly, do we expect the scripts used by customer scripts/jobs run repeatedly? I believe most scripts are not, but if we do have some, we'll need to notify users about the backward compatibility.

peternied commented 2 years ago

How should I handle deprecating functionality? Deprecating existing functionality is a normal part of software development and is often required to make forward progress. When you deprecate part of your public API, you should do two things: (1) update your documentation to let users know about the change, (2) issue a new minor release with the deprecation in place. Before you completely remove the functionality in a new major release there should be at least one minor release that contains the deprecation so that users can smoothly transition to the new API.

From https://semver.org/#how-should-i-handle-deprecating-functionality

We have not done steps (1), (2) for the security plugin, has this been done elsewhere?

sean-zheng-amazon commented 2 years ago

Good point and understand, but please note that this campaign is not about public API changes. It's about supporting files that not supposed to be used directly and programmingly by customers, so the risk is much lower. That being said, if you do notice anything that being used by customers as a dependency, please raise it up here so we can discuss.

cliu123 commented 2 years ago

Looking at the current status of this tasks on all plugins and considering the time and effort this change would take, this will be a better candidate for 3.0.0 as a breaking change. We can improve the organization-wide consistency like this over time, and this won't be done in one run.

dblock commented 2 years ago

I vote for 3.0 for breaking changes, but I think some other changes that aren't breaking can go into 2.0. I think securityadmin.sh is as close to an interface as it gets unfortunately, as it's used directly :( I'd prefer if we didn't deprecate its location, but the whole script, in favor of a well organized and documented admin tool.

CEHENKLE commented 2 years ago

Lemme ask this a different way -- Is the moving of ./bin: executables the only thing we think is truly breaking?

I'm with dB -- the wild wild west we have right now makes it really hard for someone to contribute to plugins, because you have to invest time figure out where things for each one. So if we have to break this into 2.0/3.0 changes we can do it, but let's not throw the towel in on the whole effort because it causes real pain.

xinlamzn commented 2 years ago

Agree that we should keep securityadmin.sh unchanged as it is close to an interface. For other plugins, I would like to see what changes may come with breaking changes and discuss them case by case.

peternied commented 2 years ago

Is the moving of ./bin: executables the only thing we think is truly breaking?

For the security plugin, the other changes to folder structure are not as risky. We will implement them for the 2.0.0 release. Security plugin added the deprecation notice on its impacted tools in preparation of this change in 3.0.

peterzhuamazon commented 2 years ago

Talked to @sean-zheng-amazon and this one can be closed for now. Thanks.