-
Notifications
You must be signed in to change notification settings - Fork 145
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
Permit passing config object via env vars #461
Permit passing config object via env vars #461
Conversation
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.
Thank you for the suggestion.
Indeed, it is not supported to pass objects via ENV or CLI and it is reflected as such in the docs and config.schema.js.
If this works reliably, this should be reflected in the docs and in the config.schema.js.
(Seeing that this PR is not a trivial one-liner as I had initially hoped, I've updated the description in hopes of adding more clarity to what is being changed and why) |
Hi @alukach I checked this again, but I'm not sure how this allows supporting objects. I only see changes in the Docker environment, but not generally in the non-Docker part of STAC Browser. As such I'm not sure how this is meant to work?! Can you provide an example, please? |
Thanks. This looks reasonable, but I don't see any effect. For example, Edit: On the other hand I can't currently provide anything via CLI parameter, it seems. Hmm... => #529 |
@m-mohr For testing, I've been using
Note the rendered auth button:
Not sure if this is the exact reason for your issues, but you should wrap the JSON in single quotes rather than double quotes. |
@m-mohr I've removed bd68d93 from this PR as I have been unable to replicate this issue. With the following command:
I can see that the trace ID is correctly set on all outgoing requests (I use that header to conform with the |
Something must be off on my machine, I can't provide anything via CLI right now. Might be a Windows thing or something is broken in a specific npm/node/vue-cli version. Need to investigate. Can't really test this PR right now. But good to know that this is working for you. bd68d93 came from the master branch. |
Okay, I can confirm that this works. It is an npm bug on Windows that prevented me from testing this. I didn't test the Docker implementation as I'm not using Docker. Thank you for the contribution @alukach! |
What I'm changing
If a config value is an object, the tooling to convert it from an environment var to JS will keep it as a string (see #460).
This PR will instead retain its state as a JSON object.
This will allow the following configurations to be correctly set via environment variables:
How I did it
The inability to pass in objects via ENV vars was caused by the fact that when we parse the
SB_*
environment values based on their type as specified in theconfig.schema.json
; however, we don't specifically handle theobject
type so it is run through the catch-allsafe_echo
code:stac-browser/config.schema.json
Lines 225 to 236 in 108947e
stac-browser/docker/docker-entrypoint.sh
Lines 58 to 92 in 108947e
This keeps it as a string:
stac-browser/docker/docker-entrypoint.sh
Lines 1 to 2 in 108947e
However, if we permit configurations of type "object" to encoded directly into the JSON via the
object()
function in the bash script, we can use them as standard configuration.How you can test this
Before
On the
main
branch:docker build -t stac-browser
docker run -it -e SB_authConfig='{"foo": "bar"}' --name stac-browser stac-browser
docker exec stac-browser cat /usr/share/nginx/html/config.js
You should see an object where the
authConfig
property is a string rather than an object:After
Following the above steps on this branch, you should see now see an object where the
authConfig
property is an object:closes #460