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

Copy the OpenSearch-Dashboards src package to cache for plugin build reuse #815

Closed
wants to merge 1 commit into from

Conversation

Tengda-He
Copy link
Contributor

Description

Copy the OpenSearch-Dashboards git package to ~/.cache folder when building the OpenSearch-Dashboards component. Also create the load_dashboard_from_cache function for plugin build to reuse OpenSearch-Dashboards source code. This way allows individual plugin to build independently, #606

Test:
verify locally that build OpenSearch-Dashboards plugin independently with fetching core from cache run successfully.
ex: ./build.sh manifests/1.1.0/opensearch-dashboards-1.1.0.yml --component=alertingDashboards

Issues Resolved

#812

Check List

  • 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.

@Tengda-He Tengda-He requested review from kavilla and tianleh October 27, 2021 00:28
@kavilla
Copy link
Member

kavilla commented Oct 27, 2021

Will we worry about size of the cache?

ONBOARDING.md Outdated
@@ -8,7 +8,7 @@ This document describes steps to onboard a new plugin to release workflow for co

2. Create a `scripts/build.sh` if you have specific requirements that are not covered by the [default build.sh script](/scripts/default/build.sh) and commit it to your repository.

3. Ensure your `build.sh` reads and passes along both `-Dbuild.snapshot=` and `-Dopensearch.version=` flags. Snapshot builds should produce a -SNAPSHOT tagged artifact for example `opensearch-plugin-1.1.0.0-SNAPSHOT.zip` where a release build of the same component would produce `opensearch-plugin-1.1.0.0.zip`.
3. Ensure your `build.sh` reads and passes along both `-Dbuild.snapshot=` and `-Dopensearch.version=` flags. Snapshot builds should produce a -SNAPSHOT tagged artifact for example `opensearch-plugin-1.1.0.0-SNAPSHOT.zip` where a release build of the same component would produce `opensearch-plugin-1.1.0.0.zip`. If build a opensearch-dashboards plugin be sure to have the opensearch-dashboards core already in cache.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: If building a OpenSearch Dashboards plugin*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, will update

@@ -20,6 +20,19 @@ function usage() {
echo -e "-h help"
}

function load_opensearch_dashboards_from_cache() {
if [ ! -d ../OpenSearch-Dashboards ]; then
echo "No OpenSearch-Dashboards folder found, try to load from cache"
Copy link
Member

Choose a reason for hiding this comment

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

Nit: trying

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

echo "Copy OpenSearch-Dashboards from cache"
cp -r ~/.cache/opensearch-project/$VERSION/OpenSearch-Dashboards/ ../../
else
echo "No correct version OpenSearch-Dashboards found from cache, please build OpenSearch-Dashboards first"
Copy link
Member

Choose a reason for hiding this comment

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

Nit: show we just instead give them the command to run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added cmd.

@@ -67,6 +80,7 @@ PLUGIN_FOLDER=$(basename "$PWD")
PLUGIN_NAME=$(basename $(dirname "$PWD"))
# TODO: [CLEANUP] Needed OpenSearch Dashboards git repo to build the required modules for plugins
# This makes it so there is a dependency on having Dashboards pulled already.
load_opensearch_dashboards_from_cache
Copy link
Member

Choose a reason for hiding this comment

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

With this introduced, shall "rm -rf ../../OpenSearch-Dashboards/plugins/$PLUGIN_FOLDER" be changed to delete the whole "OpenSearch-Dashboards" dir?

Copy link
Member

@kavilla kavilla Oct 27, 2021

Choose a reason for hiding this comment

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

If we do that probably shouldn't check if that folder exists in the function. But I think it's worth just deleting the PLUGIN_FOLDER and keeping the OpenSearch Dashboards folder around just because it will keep inline with the rest of the project since all the repos exist locally.

@Tengda-He Tengda-He requested a review from dblock October 27, 2021 01:16
@Tengda-He
Copy link
Contributor Author

currently the opensearchdasbhoards is about 196 mb, I think it is not a big issue yet about the cache size

…ilding the OpenSearch-Dashboards component. Also create the load_dashboard_from_cache function for plugin build to reuse OpenSearch-Dashboards source code. This way allows individual plugin to build independently, opensearch-project#606

Signed-off-by: Tengda He <tengh@amazon.com>
@dblock
Copy link
Member

dblock commented Oct 27, 2021

In #755 (comment) we decided not to go this route, what changed?

@Tengda-He
Copy link
Contributor Author

Tengda-He commented Oct 27, 2021

had a offline sync with kavilla after some investigation, thinking the right solution should be letting plugin owner figure out what modules are required from OSD for plugin build. The current situation is we have many plugins, and each has a different dependency hierarchy with OSD modules. (And some plugin have import module with relative path pointing to OSD src folder). This makes it quite hard to develop a fit-all solution from OSD side. Prefer to hand this work to plugin owner to achieve.

we want to fallback to this temp solution to unblock the issue

@gaiksaya
Copy link
Member

Have we thought about this option https://www.npmjs.com/package/local-npm ? Seems like maven local option

@dblock
Copy link
Member

dblock commented Oct 27, 2021

we want to fallback to this temp solution to unblock the issue

Because these scripts are going to be eventually moved into each individual repo, we will lose track of this workaround.

The original issue is to make it easier for developers to debug failing builds. It's not a big issue at all. Since this is not ideal, I therefore recommend closing this without merging.

Other simpler options to help with the original issue could be to allow doing ./build.sh manifest.yml --component "OpenSearch Dashboards,alerting-dashboards" for example, without any hacks.

@Tengda-He
Copy link
Contributor Author

close this PR, will pursue other solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants