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

[Decouple] Allow plugin manifest config to define semver compatible OpenSearch plugin and verify if it is installed on the cluster #4612

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

manasvinibs
Copy link
Member

@manasvinibs manasvinibs commented Jul 24, 2023

Description

This PR is an enhancement to the already existing plugin manifest ability to allow defining backend OpenSearch plugin dependencies. As part of decoupling Dashboards with OpenSearch Dashboards plugins and then with OpenSearch, we need a way to establish Dashboards plugin dependency with its counter part.
This PR adds following changes -

  • Modifying existing manifest config key requiredOpenSearchPlugins to define list of OpenSearch Plugin object and its version compatibility with that plugin.
  • In the first iteration we can define version range in semver format only.
  • Compatible version defined in the manifest should be a valid version range.
  • Validates if version compatible plugin is installed on the cluster or not.
  • In this initial implementation, we are only logging the warning if plugin is not available and does not fail to start.

In the follow-up PRs,

  • Add ability to disable Dashboards plugin when its counterpart is not installed on the cluster.
  • Export function/API that plugins can extend to decide what they want to do when their OpenSearch counterpart plugin is not installed or is incompatible.

Issues Resolved

#4611

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@codecov
Copy link

codecov bot commented Jul 24, 2023

Codecov Report

Merging #4612 (bc80e28) into main (754c78d) will increase coverage by 0.02%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##             main    #4612      +/-   ##
==========================================
+ Coverage   66.28%   66.31%   +0.02%     
==========================================
  Files        3322     3322              
  Lines       63923    63933      +10     
  Branches    10117    10121       +4     
==========================================
+ Hits        42371    42395      +24     
+ Misses      19061    19050      -11     
+ Partials     2491     2488       -3     
Flag Coverage Δ
Linux_1 34.84% <ø> (ø)
Linux_2 55.15% <95.65%> (+0.01%) ⬆️
Linux_3 43.38% <0.00%> (-0.01%) ⬇️
Linux_4 35.09% <0.00%> (-0.01%) ⬇️
Windows_1 34.85% <ø> (?)
Windows_2 55.12% <95.65%> (+0.01%) ⬆️
Windows_3 43.38% <0.00%> (?)
Windows_4 35.09% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/core/server/plugins/types.ts 100.00% <ø> (ø)
...server/plugins/discovery/plugin_manifest_parser.ts 84.72% <93.75%> (+0.34%) ⬆️
src/core/server/plugins/plugin.ts 100.00% <100.00%> (ø)
src/core/server/plugins/plugins_system.ts 90.52% <100.00%> (+0.20%) ⬆️

... and 3 files with indirect coverage changes

@manasvinibs manasvinibs added the enhancement New feature or request label Jul 24, 2023
@joshuarrrr
Copy link
Member

@kavilla Assigned you as primary reviewer.

Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

looks good I would just update this one to be the string:string and good to merge.

JSON.stringify({
id: 'test-invalid-version-type-plugin',
version: '7.0.0',
requiredEnginePlugins: { 'invalid-plugin-version-range': 2 },
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought semver was able to convert this to ^2.0.0; no?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm doing semver.coerce when reading the version from engine, but this is coming from plugin manifest file and we still expect plugin users to write it in actual semver format. Hence there is a validation check on if the defined version is valid semver range or not.

…lugin and verify if it is installed on the cluster

Signed-off-by: Manasvini B Suryanarayana <manasvis@amazon.com>
Copy link
Collaborator

@AMoo-Miki AMoo-Miki left a comment

Choose a reason for hiding this comment

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

Thanks

Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

Pretty cool! I think if there is any clean ups or changes to how we want to do it it can be done in a follow-up but I don't see anything right away. So let's let it bake in main for a little.

@manasvinibs manasvinibs merged commit 0188087 into opensearch-project:main Aug 24, 2023
54 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 24, 2023
…lugin and verify if it is installed on the cluster (#4612)

Signed-off-by: Manasvini B Suryanarayana <manasvis@amazon.com>
(cherry picked from commit 0188087)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
manasvinibs pushed a commit that referenced this pull request Aug 29, 2023
…lugin and verify if it is installed on the cluster (#4612) (#4817)
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.

4 participants