Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove patch version check for plugin compatibility #6837

Conversation

abseth-amzn
Copy link
Contributor

Signed-off-by: Abhilasha Seth [email protected]

Description

As a first step towards supporting semVer range of compatible versions for plugins, we are starting with relaxing the patch version check (for default compatibility range).

Issues Resolved

#1707

Check List

  • New functionality includes testing.
    • New tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.action.admin.cluster.node.tasks.ResourceAwareTasksTests.testTaskResourceTrackingDuringTaskCancellation

@codecov-commenter
Copy link

codecov-commenter commented Mar 26, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (ed1c8b7) 71.34% compared to head (2d82f13) 70.68%.

❗ Current head 2d82f13 differs from pull request most recent head 4b2dffc. Consider uploading reports for the commit 4b2dffc to get more accurate results

Files Patch % Lines
...in/java/org/opensearch/plugins/PluginsService.java 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #6837      +/-   ##
============================================
- Coverage     71.34%   70.68%   -0.66%     
- Complexity    58964    59163     +199     
============================================
  Files          4890     4810      -80     
  Lines        277477   283488    +6011     
  Branches      40308    40877     +569     
============================================
+ Hits         197961   200397    +2436     
- Misses        62976    66651    +3675     
+ Partials      16540    16440     -100     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dblock
Copy link
Member

dblock commented Mar 27, 2023

We need to discuss whether relaxing plugin compatibility versions is a breaking change. If it is, then we need this behavior to be configurable and default to false.

@saratvemulapalli wdyt?

Copy link
Member

@saratvemulapalli saratvemulapalli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets also take care of adding tests to: https://github.com/opensearch-project/OpenSearch/blob/main/distribution/tools/plugin-cli/src/test/java/org/opensearch/plugins/InstallPluginCommandTests.java

This internally uses PluginsService.java verifyCompatibility.

@saratvemulapalli
Copy link
Member

We need to discuss whether relaxing plugin compatibility versions is a breaking change. If it is, then we need this behavior to be configurable and default to false.

@saratvemulapalli wdyt?

Good question, looking at Semver[1] MINOR version when you add functionality in a backwards compatible manner.
This feature is backward compatible since we are relaxing patch version, our installation APIs will not break.

That said, for developers who are using this feature we really want to "caution" that its only for users "who know what they are doing". Because there are risks associated with relaxing patch versions and might up into runtime problems.
Gating it by a feature flag would help lower the risk.

@abseth-amzn we could add another parameter plugin-descriptor.properties[1] file and enable devs to configure this feature and set it false by default if it doesn't exist and open an issue in https://github.com/opensearch-project/documentation-website/ to document details of this feature.

[1] https://github.com/opensearch-project/OpenSearch/blob/main/buildSrc/src/main/resources/plugin-descriptor.properties

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@opensearch-trigger-bot opensearch-trigger-bot bot removed the stalled Issues that have stalled label Jul 28, 2023
@abseth-amzn abseth-amzn force-pushed the remove-patch-version-check branch from c9b2dc5 to 19fc7d5 Compare August 3, 2023 06:39
@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2023

Gradle Check (Jenkins) Run Completed with:

@opensearch-trigger-bot
Copy link
Contributor

Compatibility status:


> Task :checkCompatibility
Checking compatibility for: https://github.com/opensearch-project/reporting.git with ref: main
Incompatible components: [https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/ml-commons.git]
Compatible components: [https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/reporting.git]

BUILD SUCCESSFUL in 31m 18s

@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2023

Gradle Check (Jenkins) Run Completed with:

@abseth-amzn abseth-amzn force-pushed the remove-patch-version-check branch from 31f210a to 0a1b8f1 Compare August 8, 2023 10:42
@opensearch-trigger-bot
Copy link
Contributor

Compatibility status:



> Task :checkCompatibility
Incompatible components: [https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/security-analytics.git]
Compatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git]

BUILD SUCCESSFUL in 29m 13s

@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2023

Gradle Check (Jenkins) Run Completed with:

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm opposed to this change as is (a boolean that says that a plugin is compatible with patch versions of OpenSearch). I tried to say it in #6837 (review) above. The tl;dr is:

  1. If a plugin is authored for OpenSearch 2.8+, a boolean will say it's also compatible with 2.7, but it will not work.
  2. A boolean cannot evolve into a semver range in the future, so it's not a good foundation to build upon.

I would be ok with a semver compatible range that can be declared by a plugin, even if only a subset of it is implemented. As a plugin author I'd like to write ">= 2.5", "~> 2.5", etc., in the plugin manifest. I would be ok if the implementation doesn't support more complex syntax in v1, but I want to make sure we can add support for any semver syntax in the future without having to change the developer/plugin interface.

Signed-off-by: Abhilasha Seth <[email protected]>

Signed-off-by: Abhilasha Seth <[email protected]>
Signed-off-by: Abhilasha Seth <[email protected]>
Signed-off-by: Abhilasha Seth <[email protected]>
Signed-off-by: Abhilasha Seth <[email protected]>
Signed-off-by: Abhilasha Seth <[email protected]>
Signed-off-by: Abhilasha Seth <[email protected]>
Copy link
Contributor

Compatibility status:

Checks if related components are compatible with change 4b2dffc

Incompatible components

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/performance-analyzer.git]

Copy link
Contributor

❌ Gradle check result for 4b2dffc: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@dblock
Copy link
Member

dblock commented Nov 28, 2023

I'm opposed to this change as is (a boolean that says that a plugin is compatible with patch versions of OpenSearch).

@abseth-amzn Please address my comment.

@abseth-amzn
Copy link
Contributor Author

Closing in favor of #11441

@abseth-amzn abseth-amzn closed this Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants