Skip to content

Conversation

@leifjones
Copy link

@leifjones leifjones commented Nov 30, 2020

Description

  • Bring in Nginx configuration approach from swagger-api/swagger-ui repository.
  • Update Dockerfile appropriately
  • Add flags within index.html to allow configurator/index.js to attempt to replace text should users choose to. (I imagine this might address an issue, but I have not searched for such.)
  • Modify configurator/translator.js to match the current values within index.html's window.onload

@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?

  1. docker build .
  2. docker run -e BASE_URL='/swagger-editor' -p 8001:8080 86547a8f42b1
  3. Navigate to http://localhost:8001/swagger-editor and see that the page loads

Checklist

My PR contains...

  • No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes to a public API (config options, System API, major UI change, etc).
  • are breaking changes to a private API (Redux, component props, utility functions, etc.).
  • are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
  • are not breaking 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

  • My changes do not require a change to the project documentation.
  • My changes require a change to the project documentation.
  • If yes to above: I have updated the documentation accordingly.

Automated tests

  • My changes can not or do not need to be tested.
  • My changes can and should be tested by unit and/or integration tests.
  • If yes to above: I have added tests to cover my changes.
  • If yes to above: I have taken care to cover edge cases in my tests.
  • All new and existing tests passed.

^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.

Dockerfile changes towards.
Very likely not yet addressing needed nginx changes.
@leifjones
Copy link
Author

leifjones commented Dec 1, 2020

Dockerfile seemingly doesn't work locally. I'm surprised tests pass in that case.

This may indicate an need for an additional workflow check in that area.

'Twas a line endings issue.

@leifjones leifjones changed the title Environment variable addition. Update Dockerfile approach. Allow for specification of BASE_URL via container environment variable Dec 7, 2020
@leifjones leifjones marked this pull request as ready for review December 7, 2020 15:02
@@ -0,0 +1,113 @@
const standardVariables = {
Copy link
Author

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: {
Copy link
Author

Choose a reason for hiding this comment

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

Seems worth including insofar as it may help with #1969 and #1510

Copy link
Author

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
Copy link
Author

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.

@leifjones
Copy link
Author

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.

@leifjones
Copy link
Author

Closing this in favor of a cleaner PR: #2281

@leifjones leifjones closed this Dec 12, 2020
@leifjones leifjones deleted the Add-support-for-dynamic-context-path branch July 6, 2023 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for dynamic context path

2 participants