-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add docker image to repo #6
Conversation
ecf6f8d
to
95e3b44
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep it simple and don't import the complexity of the main Datahub
tools/docker/Dockerfile
Outdated
|
||
ARG APP_NAME="search" | ||
ENV APP_NAME=datahub | ||
ENV GN4_API_URL "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need all this ENV, there used for the generic datahub, but might not be useful for MEL.
I don't even think that we need the config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Web probably don't need an entrypoint, do we ?
apps/datahub/project.json
Outdated
"executor": "nx:run-commands", | ||
"options": { | ||
"commands": [ | ||
"nx build datahub", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, I've renamed the app to mel-datahub
(to be unique) in the last PR that was merged. The name might need to be adapted here after rebase.
caf36a1
to
cf29540
Compare
tools/docker/docker-entrypoint.sh
Outdated
# no conf file; use env variables to tweak app config | ||
echo "[INFO] No custom configuration file found at ${CONFIG_OVERRIDE_FILE_PATH}" | ||
# Modify the GN4 url and proxy path based on env variables (if defined) | ||
if [ ! -z "${GN4_API_URL}" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering where we should set the GN4_API_URL
.
- Do we really need a config? Not sure if we already handle reading the config or if we can reuse the gn-ui mechanism, btw.
- Should we set it via the env var when building the image?
- Should we set it in the
proxy-config.js
(which I have changed to localhost for dev and e2e tests).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cf my previous comment, I don't think that we need an entrypoint at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need it for the header &/or custom scripts at less ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so I saw a problem where there is none, as the proxy-config.js
is not used in prod, but the datahub points to /geonetwork
, thanks for the explanation @f-necas !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @f-necas ! LGTM now 🙂
#!/bin/bash | ||
|
||
# Executing custom scripts located in CUSTOM_SCRIPTS_DIRECTORY if environment variable is set | ||
if [[ -z "${CUSTOM_SCRIPTS_DIRECTORY}" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What kind of script do you execute from here ?
Add the header for instance ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By now, yes, we just add the header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't the header be added directly within the application instead ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could and would be better but should be another ticket.
No description provided.