bamaer commented on PR #8723:
URL: https://github.com/apache/hop/pull/8723#issuecomment-5948246133
@mattcasters, thanks for picking this up. Installing marketplace plugins in
a container is a real gap, and following the `HOP_DRIVERS_DOWNLOAD` pattern
makes it easy to find for people who already use the images. Building on the
existing marketplace repository code pays off: basic auth, bearer tokens and
the `HOP_MARKETPLACE_*` credentials all worked straight away against an
authenticated repo in my tests. Auth failures come with clear, actionable
messages. Your hopper-edw example installed on the first try. The new
`--repo-url` / `--username` / `--auth-type` flags on `hop marketplace install`
are a nice addition in their own right.
I spent some time testing this end to end, and I think it needs another
round before it's ready, mostly in the Docker integration rather than the CLI.
Could we move it to draft while that's sorted out? Details below.
**What I tested:** I used the PR's marketplace jar in a client build and ran
the `install_marketplace_plugins` function from `load-and-execute.sh`:
- Installing `org.hopper:hopper-edw:0.10.0` from the data-hopper Nexus works.
- Against an authenticated Maven repo, basic auth via `HOP_PLUGINS_REPO_*`,
a bearer token, and the global `HOP_MARKETPLACE_*` credentials all work.
- I timed the install in containers with CPU limits.
- I didn't build the Docker images from this branch.
**Blockers**
- **Web image: `run-web.sh` can't run `hop`.** In `webapps/ROOT`, `./hop
marketplace list` fails with `ClassNotFoundException: org.apache.hop.hop.Hop`.
Its classpath is `lib/core/*`, which doesn't exist there, and neither
`unified.Dockerfile` nor `web.Dockerfile` rewrites it (only `hop-*.sh`). So
setting `HOP_PLUGINS_DOWNLOAD` makes the web container exit with 8.
`HOP_DRIVERS_DOWNLOAD` has the same problem today. Also, `lib/core` jars would
be written to `/usr/local/tomcat/lib/core`, which isn't on the webapp
classpath. Suggest dropping the web part, or fixing the `hop` classpath and
installing `lib/core` jars into `WEB-INF/lib`.
- **Every container start installs everything again, and there's no way to
cache it.** Each start resolves, downloads, unzips and copies every plugin
again: `install` ignores the receipt, while `apply` checks it. Installs always
go to `/opt/hop` (`plugins/`, `lib/core/`), so keeping them means mounting over
the image's own plugins, and a read-only root filesystem breaks the install.
Measured for hopper-edw (78 MB), per container start:
| CPU limit | Hop startup alone | Full install |
|---|---|---|
| 4 CPUs | 3.5 s | 11.6 s |
| 1 CPU | 6.8 s | 15.2 s |
| 0.5 CPU | 13.3 s | 24.4 s |
That's one JVM per plugin, before Hop itself starts. Since the script
ships in the stock image, this becomes the default path. For
one-container-per-run use (hop-run from Airflow, Kubernetes Jobs) every run
pays it in full, and the plugin repo becomes a dependency of every run.
Suggested fix:
- Skip a plugin when its receipt version matches and its files are present.
- Allow installing into a mounted folder listed in
`HOP_PLUGIN_BASE_FOLDERS`.
- Install all coordinates with one `hop marketplace install` call.
- Document building a derived image (`RUN hop marketplace install
--repo-url …`) as the recommended production route.
**Should fix**
- **The password is passed as `--password=…` and is visible in `ps` /
`/proc/*/cmdline`.** The same script writes the hop-server config to a file to
avoid exactly that. Suggest passing it through the existing
`HOP_MARKETPLACE_<ID>_PASSWORD` / `_TOKEN` variables instead of the CLI flag.
- **`HOP_PLUGINS_MAVEN_REPO` is used exclusively, as the forced repo.** This
rules out mixing Apache optional plugins with private ones, which the script
header's own example does. Suggest making it the preferred repo but keeping the
usual fallback to the other repos.
- **Short names fail with a misleading error on repos that can't be
browsed.** `acme-plugin:1.0.0` against a plain Maven repo silently became
`org.apache.hop:acme-plugin:1.0.0` and returned a 404. The repo type is guessed
from the URL and can't be overridden for the ad-hoc repo. Suggest failing with
a clear "use group:artifact:version" error, and adding a repo-type setting.
- **The ad-hoc repo id is hard-coded as `adhoc-repo`.** The
`HOP_PLUGINS_REPO_ID` proposed in #8722 is missing, so per-repo credential
variables only work under the accidental names `HOP_MARKETPLACE_ADHOC_REPO_*`.
Suggest restoring the repo id variable and documenting the per-repo credential
variables.
- **The new variables aren't declared in the image.** `HOP_DRIVERS_*` is
declared with `ENV` in both stages of `unified.Dockerfile`; the `HOP_PLUGINS_*`
variables should follow the same pattern.
- **Tests only cover an anonymous download.** Suggest adding one against a
repo that requires authentication: basic, token, and one that rejects anonymous
requests.
- **Docs.** "Available from Hop version 2.19" should say 2.20. The
`HOP_MARKETPLACE_PLUGINS`, `HOP_MARKETPLACE_REPO_URL` and
`HOP_MARKETPLACE_ENV_FILE` aliases should be documented or dropped. Also say
that installed plugins aren't kept between containers.
The new `--repo-url`, `--username` and `--auth-type` flags on `hop
marketplace install` are useful on their own, for installing in a Dockerfile at
build time. That part could be split out and merged first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
