-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Allow for specification of BASE_URL via container environment variable #2257
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
Allow for specification of BASE_URL via container environment variable #2257
Conversation
Dockerfile changes towards. Very likely not yet addressing needed nginx changes.
|
'Twas a line endings issue. |
Also match translator.js to default in index.html
| @@ -0,0 +1,113 @@ | |||
| const standardVariables = { | |||
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.
If there's a reason to reduce which of these are being added, please let me know.
| type: "boolean", | ||
| name: "useUnsafeMarkdown" | ||
| }, | ||
| OAUTH2_REDIRECT_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.
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.
This may be applicable to the functionality being added with #2073
| fi | ||
|
|
||
| ## Adding env var support for swagger file (json or yaml) | ||
| if [[ -f "$SWAGGER_FILE" ]]; 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.
I have not yet tested whether this preserves or breaks this docker run -d -p 80:8080 -v $(pwd):/tmp -e SWAGGER_FILE=/tmp/swagger.json swaggerapi/swagger-editor functionality.
|
I acknowledge there is the tangentially related PR #2073 . I anticipate no conflict. In fact, before I was considering adding that change (adding the oauth2 redirect html file) to this branch. |
|
Closing this in favor of a cleaner PR: #2281 |
Description
@fehguy Because you're listed as maintainer for the Dockerfile, I want to let you know about this PR. (To others, though, please note that this pattern used is identical to what is in the analogous Dockerfile for which fehguy is also listed as maintainer.)
Motivation and Context
This adds the option to specify a BASE_URL at which to serve the application. As is done with Swagger UI (see it's Installation documentation), the above PR now allows specification of a BASE_URL. This allows the editor to be served at a different location (e.g.
/swagger-editor), which can enable this to be served at a single URL routed via a Kubernetes Ingress object.Fix: #1956
I had hoped to also address #1969 by simply copying in that file to this repository, but I am not yet prepared to test that responsibly. It is mentioned in the js/js.map bundles.)
This may or may not also help with #1510. I hope that it does, but that's also outside the scope of current focus.
How Has This Been Tested?
docker build .docker run -e BASE_URL='/swagger-editor' -p 8001:8080 86547a8f42b1Checklist
My PR contains...
src/is unmodified: changes to documentation, CI, metadata, etc.)package.json)My changes...
^ I don't know of any breaking changes. Admittedly, there are non-trivial changes to the container image. Though, note that it's very similar to what is in place in Swagger Editor.
Documentation
Automated tests
^Up for consultation. I notice early pushes of this PR passed all checks even though the built image wasn't yet functional. If anyone who knows of approaches to test built docker images - that are super easy, please let me know. Otherwise, I'd ask that you please help more concretely.