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

Provide default directory for js plugins #3070

Merged
merged 2 commits into from
Jan 11, 2023

Conversation

devinrsmith
Copy link
Member

@devinrsmith devinrsmith commented Nov 6, 2022

I'd like to consider moving towards a model where "if you follow directory conventions, you don't need to explicitly set configuration properties".

As a motivating example, I'd like consumers of our docker images to install a JS plugins without needing to explicitly include a configuration property (file or system property).

# syntax=docker/dockerfile:1.4

FROM ghcr.io/deephaven/web-plugin-packager:latest as build
RUN ./pack-plugins.sh @deephaven/js-plugin-matplotlib

FROM ghcr.io/deephaven/server:latest
RUN pip install deephaven-plugin-matplotlib
COPY --link --from=build js-plugins/ /opt/deephaven/config/js-plugins/

As it stands now, the user would either need to explicitly add deephaven.prop:

includefiles=dh-defaults.prop

deephaven.jsPlugins.resourceBase=/opt/deephaven/config/js-plugins
COPY --link deephaven.prop /opt/deephaven/config/deephaven.prop

or, add a system property (which I want to try and discourage as much as possible):

ENV START_OPTS="-Ddeephaven.jsPlugins.resourceBase=/opt/deephaven/config/js-plugins"

This example is based off of deephaven-examples/deephaven-matplotlib-base#3

@devinrsmith
Copy link
Member Author

Would be able to update and simplify https://github.com/deephaven/deephaven.io/pull/1947

Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

@devinrsmith Can we log some feedback on what JS plugin directory was loaded? Right now it doesn't print anything, and as someone trying it out I don't know what directory it's checking, or what directory was loaded, etc.
For example, if I'm running:

 DEEPHAVEN_DATA_DIR=/Users/bender/tmp/deephaven-data ./gradlew server-jetty-app:run

What directory is being used for js-plugins? I have no idea, I tried putting a manifest.json in /Users/bender/tmp/deephaven-data/js-plugins/manifest.json and at /Users/bender/tmp/deephaven-data/storage/js-plugins/manifest.json, and it didn't seem to pick it up. I really don't know where this is actually looking.

@devinrsmith
Copy link
Member Author

devinrsmith commented Dec 19, 2022

This is a fine feature request, irrespective of this PR IMO.

Y̶o̶u̶ ̶c̶a̶n̶'̶t̶ ̶p̶a̶s̶s̶ ̶e̶n̶v̶i̶r̶o̶n̶m̶e̶n̶t̶ ̶v̶a̶r̶i̶a̶b̶l̶e̶s̶ ̶t̶h̶r̶o̶u̶g̶h̶ ̶̶.̶/̶g̶r̶a̶d̶l̶e̶w̶ ̶.̶.̶.̶̶ ̶b̶t̶w̶,̶ ̶t̶h̶a̶t̶'̶s̶ ̶n̶o̶t̶ ̶b̶e̶e̶n̶ ̶a̶ ̶f̶e̶a̶t̶u̶r̶e̶ ̶w̶e̶'̶v̶e̶ ̶s̶u̶p̶p̶o̶r̶t̶e̶d̶.̶ ̶

Edit: my bad, you can pass environment variables through ./gradlew ..., so DEEPHAVEN_DATA_DIR=/my/path ./gradlew ... does work. (Note: START_OPTS and JAVA_OPTS are bash script environment variables not applicable to the java process, so they would need a different approach, see #3220)

@devinrsmith
Copy link
Member Author

@mofojed - if you try your previous run with DEEPHAVEN_CONFIG_DIR instead of DEEPHAVEN_DATA_DIR, I think it will work.

Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Setting DEEPHAVEN_CONFIG_DIR instead of DEEPHAVEN_DATA_DIR worked.

@devinrsmith devinrsmith merged commit 6cf8a12 into deephaven:main Jan 11, 2023
@devinrsmith devinrsmith deleted the js-plugins-default-directory branch January 11, 2023 18:49
@github-actions github-actions bot locked and limited conversation to collaborators Jan 11, 2023
@deephaven-internal
Copy link
Contributor

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

Successfully merging this pull request may close these issues.

4 participants