Skip to content
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

Feature/docker volumes #3652

Closed
wants to merge 8 commits into from

Conversation

guidsen
Copy link

@guidsen guidsen commented Sep 10, 2017

Regarding my suggestion on #3348 (comment).

This Pull Request will add Docker volume support. Let me know if there are any questions.

README.md Outdated
You can pull a pre-built docker image of the swagger-ui directly from Dockerhub:

```
docker pull swaggerapi/swagger-ui
docker run -p 80:8080 swaggerapi/swagger-ui
docker run -p 1337:8080 swaggerapi/swagger-ui
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep it at port 80.

Copy link
Author

Choose a reason for hiding this comment

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

👍

@webron webron requested a review from ponelat September 11, 2017 08:52
@guidsen
Copy link
Author

guidsen commented Sep 12, 2017

I've updated the PR based on your feedback @webron

@webron
Copy link
Contributor

webron commented Sep 12, 2017

Thanks, @guidsen. I'm waiting for @ponelat to review as well.

sed -i "s|http://example.com/api|$API_URL|g" $INDEX_FILE
fi

if [[ -f "$SPEC_DIR/$SWAGGER_JSON" ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

Is this a duplicate if statement?


```
docker run -p 80:8080 -e SWAGGER_JSON=/foo/swagger.json -v /bar:/foo swaggerapi/swagger-ui
docker run -p 80:8080 -e "SWAGGER_JSON=swagger.json" -v /MyProject/spec/:/usr/share/nginx/html/spec/ swaggerapi/swagger-ui
Copy link
Member

Choose a reason for hiding this comment

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

mounting the whole /usr/share/.../spec is long, And given the docker run -p 80:8080 -e "SWAGGER_JSON=/spec/swagger.json" -v /MyProject/spec/:/spec/ swaggerapi/swagger-ui example below. Which only mount /spec. I feel we should combine these two.

My suggestion is we add another nginx root. That will look inside /spec and default to /spec/swagger.{yaml|json}

Something as simple as

location /spec {
    index swagger.yaml swagger.json; # The index file may/may-not work. And is only a nice to have.
    root /spec
}

This means we just need to update the URL inside the index.html file to point to /spec or /spec/$SWAGGER_JSON. And we don't need to copy files over to /usr/share/nginx/...

So the examples become...

docker run -p 80:8080 -v /MyProject/spec:/spec swaggerapi/swagger-ui # Will use /MyProject/spec/swagger.json file ( $refs will be supported too, since this is the whole folder )


docker run -p 80:8080 -v -e "SWAGGER_JSON=petstore.yaml" /MyProject/spec:/spec swaggerapi/swagger-ui # Will use /MyProject/spec/petstore.yaml

The idea is only have one volume command. Less moving parts.

@guidsen let me know your thoughts on this. Or if anything I said was vague. And thank you for your efforts!

@shockey
Copy link
Contributor

shockey commented Sep 29, 2017

Bumping this thread - @ponelat @webron, does this PR need changes?

@ponelat
Copy link
Member

ponelat commented Oct 5, 2017

@shockey There is a proposed enhancement, but I don't feel it should block anything.

@@ -21,7 +22,6 @@ replace_or_delete_in_index () {
fi
}

replace_in_index myApiKeyXXXX123456789 $API_KEY
Copy link
Contributor

Choose a reason for hiding this comment

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

It appears that this apiKey functionality was removed in this PR - am I missing something? If not, why was it removed?

@shockey
Copy link
Contributor

shockey commented Feb 26, 2018

Closing; never heard back about my review. Happy to reopen this if you're still interested in having the feature merged, @guidsen!

@shockey shockey closed this Feb 26, 2018
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.

4 participants