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

make serving path configurable #204

Closed
wants to merge 1 commit into from

Conversation

xetalr
Copy link

@xetalr xetalr commented Sep 14, 2024

Sometimes there is a need to use a custom path for Swagger UI. It is hard-coded as api-docs at the moment. Here is proposed implementation.

url: window.location.origin + "/api-docs/swagger.json",
url: window.location.toString().replace("/index.html", "/swagger.json"),
Copy link
Collaborator

@paulo-ferraz-oliveira paulo-ferraz-oliveira Sep 16, 2024

Choose a reason for hiding this comment

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

Could you explain this change, please?

Also, I can't remember, but are we testing this somewhat?

You might want to look into up_swagger.sh because there's logic for this file, there.

Copy link

Choose a reason for hiding this comment

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

I suppose here provided trying dynamically change api-docs but... this will not works as expected. Because specific window location will return specific value... Need find other approach.

Copy link

Choose a reason for hiding this comment

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

Also, I can't remember, but are we testing this somewhat?

Based on CI results - no.

Copy link

Choose a reason for hiding this comment

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

What if.. during build this project the value /api-docs/ will be replaced based on config values? I suppose it can be done in rebar.config level as well.

Copy link
Member

Choose a reason for hiding this comment

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

yeah… env variable or macro would be great for that.

Comment on lines -39 to +40
{file, StaticFiles ++ "/index.html"},
{file, Path ++ "/index.html"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

I seem to not be able to follow this change. Would you elaborate, please?

Copy link

Choose a reason for hiding this comment

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

It looks like here need use StaticFiles as it was before, because only path should be dynamic - maybe was added by mistake?

cowboy_swagger_redirect_handler,
{file, StaticFiles ++ "/index.html"},
{file, Path ++ "/index.html"},
Copy link

Choose a reason for hiding this comment

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

Suggested change
{file, Path ++ "/index.html"},
{file, StaticFiles ++ "/index.html"},

moved_permanently(Req, State) ->
{{true, "/api-docs/index.html"}, Req, State}.
moved_permanently(Req, {file, File} = State) ->
{{true, File}, Req, State}.
Copy link

Choose a reason for hiding this comment

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

Hm... I can be wrong but maybe

Suggested change
{{true, File}, Req, State}.
Path = application:get_env(cowboy_swagger, path, "/api-docs"),
{{true, Path ++ "/" ++ File}, Req, State}.

?

@@ -119,6 +119,9 @@ Additionally, `cowboy_swagger` can be configured/customized from a `*.config` fi
%% cowboy_swagger config
{cowboy_swagger,
[
%% `path`: Path where you can access Swagger-UI. Default: `/api-docs`.
Copy link
Member

Choose a reason for hiding this comment

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

@vkatsuba I'm not sure what you wanted to add with your comment. The fact that the default value is /api-docs was already established in the original text.

@xetalr xetalr closed this Oct 4, 2024
@xetalr xetalr deleted the make_serving_path_configurable branch October 4, 2024 06:38
@elbrujohalcon
Copy link
Member

@vkatsuba I guess you're out of luck 🤷

@vkatsuba
Copy link

vkatsuba commented Oct 4, 2024

@vkatsuba I guess you're out of luck 🤷

Not really. Actually I was want to prepare something but then Mr. @paulo-ferraz-oliveira mention this PR. Give me few days please, when I have a little more free time and I will try finished this PR with mention original author and link PR's and task. Of course if @xetalr do not want finished this one or prepare new one 🙃.

@paulo-ferraz-oliveira
Copy link
Collaborator

No need for Mr. 😄 I'm younger than Brujo.

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