Add possibility to configure own path to "/api-docs/swagger.json" #207

vkatsuba · 2024-10-09T07:52:49Z

🎫 #206
🔗 Initial PR #204
🏗️ CI results https://github.com/vkatsuba/cowboy_swagger/actions/workflows/ci.yml

vkatsuba · 2024-10-09T09:34:08Z

Hey @elbrujohalcon, @paulo-ferraz-oliveira! Please, review this PR when you will has a time for it. The CI was passed except OTP 24 - it looks like some plugin require OTP 25 minimum versions, but this is another issue which was opened for this project. In any case, I will going back to end to end testing after some time, if you will keep in mind better approaches or have any opinions - please, share 😃

elbrujohalcon

As I said in the inline comment: I don't think this should be a rebar.config.script. I think it will be better off as a rebar3 pre-compile hook.

elbrujohalcon · 2024-10-09T10:47:23Z

.github/workflows/ci.yml

@@ -14,10 +14,10 @@ jobs:
    runs-on: ubuntu-22.04
    strategy:
      matrix:
-        otp_vsn: ['24', '25', '26']
+        otp_vsn: ['25', '26']


Do you mind adding '27' here, @vkatsuba ?

btw - it strange that in CI we got this issue... locally I use OTP 24 and it builded as well for me...

You probably had a cached version of the library.

Nope. Not that.
Because the libs all have version numbers…

cowboy_swagger/rebar.config

Lines 24 to 29 in 24334d8

{project_plugins,

[{rebar3_hank, "~> 1.4.0"},

{rebar3_hex, "~> 7.0.7"},

{rebar3_format, "~> 1.3.0"},

{rebar3_lint, "~> 3.1.0"},

{rebar3_ex_doc, "~> 0.2.20"}]}.

No idea what's going on.

Try running rebar3 upgrade --all. Let's see what it tells you.

CI updates as well https://github.com/vkatsuba/cowboy_swagger/actions/runs/11253721613

The rebar3 upgrade show all green

Well... rebar3 test show me that error locally.

===> Performing cover analysis... |-----------------------------------|------------| | module | coverage | |-----------------------------------|------------| | cowboy_swagger | 83% | | cowboy_swagger_handler | 73% | | cowboy_swagger_json_handler | 100% | | cowboy_swagger_redirect_handler | 100% | |-----------------------------------|------------| | total | 83% | |-----------------------------------|------------| coverage calculated from: /home/vk/work/OpenSource/git-repo/cowboy_swagger/_build/test/cover/ct.coverdata cover summary written to: /home/vk/work/OpenSource/git-repo/cowboy_swagger/_build/test/cover/index.html ===> Comparing 83 to pass rate 0 ===> Expanded command sequence to be run: [app_discovery,install_deps,lock,ex_doc] ===> Running provider: app_discovery ===> Found top-level apps: [cowboy_swagger] using config: [{src_dirs,["src"]},{lib_dirs,["apps/*","lib/*","."]}] ===> 24.3.3 satisfies the requirement for minimum OTP version 23 ===> Running provider: install_deps ===> Verifying dependencies... ===> Linking _build/default/lib/cowboy to _build/docs/lib/cowboy ===> Linking _build/default/lib/jsx to _build/docs/lib/jsx ===> Linking _build/default/lib/ranch to _build/docs/lib/ranch ===> Linking _build/default/lib/trails to _build/docs/lib/trails ===> 24.3.3 satisfies the requirement for minimum OTP version 23 ===> Linking _build/default/lib/cowlib to _build/docs/lib/cowlib ===> Running provider: lock ===> Running provider: ex_doc ===> You are using Erlang/OTP '24', but this plugin requires at least Erlang/OTP 25.

It looks like the warning related more to https://github.com/elixir-lang/ex_doc/blob/main/CHANGELOG.md... Elixir project 🤢

elbrujohalcon · 2024-10-09T10:52:12Z

rebar.config.script

@@ -0,0 +1,24 @@
+%% Reading the contents of the file
+case file:read_file("priv/swagger/swagger-initializer.js") of


I'm hesitant about implementing this as a rebar.config.script. To me it should be a pre-compile hook.
I wonder if I run rebar3 test on this project… and it changes the swagger-initializer.js… wouldn't it be included in the subsequent commit and thus pushed to Github without the placeholder?
How do we avoid that?

I was add to gitginore this file.. hope it will fix issue

Not a good idea… we do want that file downloaded when people clones the repo.

NO NO, this file is added as well and it is a part of repo. But the changes in this file should be ignored - something like that.

What if we need to make changes to this file (like the automatic updater does)?
In that case, it will need to git add -f priv/swagger/swagger-initializer.js.
I don't think that's a good idea.
But then again, I don't know how to avoid changing the file in development mode (i.e. when working on this app) but do change it when this project is a dependency of another… 🤔
A pre-compile hook will work for making the change, but it will need to be aware of the environment. I'm not sure if that's possible with rebar3. Maybe rebar3 has an environment variable, like REBAR3_PROJECT and the script can check if it's cowboy_swagger or not… or it check if its current path includes _build (in which case, it can assume it's on another project)… 🤔

Well... I have idea but not sure... if this will looks ok. So, we can use swagger-initializer.template in repo and ignore swagger-initializer.js - during run project it will be translated from template to js and just copy all data inputs from this file before make changes.

Yeah… that's not a bad idea. What do you think, @paulo-ferraz-oliveira ?

We'll need to adjust the github action to write swagger-initializer.template based on the swagger-initializer.js file that it downloads from the swagger repo.

I'm Ok with the current implementation, but maybe if I have time, just before we decide to merge I'd like to test it out locally. I don't see documentation updated or have a really easy to understand example of how the new changes would be used.

elbrujohalcon · 2024-10-09T10:56:59Z

Furthermore, @vkatsuba why don't we make the whole url configurable, instead of just the base path?
That way, people will be able to use stuff like…

window.location.origin + '/api-docs/swagger.json'; or…
https://my.other.server/api-docs/swagger.json'; or…
window.location.origin + '/my-other-path/swagger.json'; or…
window.location.origin + '/api-docs/my-other.json'

vkatsuba · 2024-10-09T10:57:31Z

As I said in the inline comment: I don't think this should be a rebar.config.script. I think it will be better off as a rebar3 pre-compile hook.

Yes, the initial approach was intended for the pre-hook, but for some reason, I wasn't able to implement it. I haven’t worked with pre-hooks before, so I might have taken the wrong approach. Let me check it again and see if I can fix it.

vkatsuba · 2024-10-09T11:01:55Z

Furthermore, @vkatsuba why don't we make the whole url configurable, instead of just the base path? That way, people will be able to use stuff like…
* `window.location.origin + '/api-docs/swagger.json'`; or…

* `https://my.other.server/api-docs/swagger.json'`; or…

* `window.location.origin + '/my-other-path/swagger.json'`; or…

* `window.location.origin + '/api-docs/my-other.json'`

Well.... I suppose for this case we need use something like
path and url or something like that, because the path value is used in few places where the end of URL can be index.html.... but... we can take a URL and parse it maybe and use specific parts where we want... OK, let's follow second approach, let's make changes for path and then we will back tho this discussion as well.

elbrujohalcon · 2024-10-09T12:00:53Z

My idea was to use something like… {pre_hooks, [{compile, "priv/update-swagger-url"}]}. Then, write a bash script or an escript (at priv/update-swagger-url) that would basically do what you did on the rebar.config.script.

…

On Wed, 9 Oct 2024 at 13:41, Viacheslav Katsuba ***@***.***> wrote: As I said in the inline comment: I don't think this should be a rebar.config.script. I think it will be better off as a rebar3 pre-compile hook. Yes, the initial approach was intended for the pre-hook, but for some reason, I wasn't able to implement it. I haven’t worked with pre-hooks before, so I might have taken the wrong approach. Let me check it again and see if I can fix it. So, @elbrujohalcon <https://github.com/elbrujohalcon> I do not work with hooks before and ... when I check documentation the hooks mostly used for bash or C and I do not has any good example how hook can be added - do you has any ideas? — Reply to this email directly, view it on GitHub <#207 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAW3WMUFB6PY2MJR5LAPCTZ2UJAJAVCNFSM6AAAAABPT42LX6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBSGA4DMOBXHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- <https://about.me/elbrujohalcon?promo=email_sig&utm_source=product&utm_medium=email_sig&utm_campaign=gmail_api&utm_content=thumb> Brujo Benavides about.me/elbrujohalcon <https://about.me/elbrujohalcon?promo=email_sig&utm_source=product&utm_medium=email_sig&utm_campaign=gmail_api&utm_content=thumb>

vkatsuba · 2024-10-09T12:03:44Z

My idea was to use something like… {pre_hooks, [{compile, "priv/update-swagger-url"}]}. Then, write a bash script or an escript (at priv/update-swagger-url) that would basically do what you did on the rebar.config.script.
…
On Wed, 9 Oct 2024 at 13:41, Viacheslav Katsuba @.> wrote: As I said in the inline comment: I don't think this should be a rebar.config.script. I think it will be better off as a rebar3 pre-compile hook. Yes, the initial approach was intended for the pre-hook, but for some reason, I wasn't able to implement it. I haven’t worked with pre-hooks before, so I might have taken the wrong approach. Let me check it again and see if I can fix it. So, @elbrujohalcon https://github.com/elbrujohalcon I do not work with hooks before and ... when I check documentation the hooks mostly used for bash or C and I do not has any good example how hook can be added - do you has any ideas? — Reply to this email directly, view it on GitHub <#207 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAW3WMUFB6PY2MJR5LAPCTZ2UJAJAVCNFSM6AAAAABPT42LX6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBSGA4DMOBXHE . You are receiving this because you were mentioned.Message ID: @.>
-- https://about.me/elbrujohalcon?promo=email_sig&utm_source=product&utm_medium=email_sig&utm_campaign=gmail_api&utm_content=thumb Brujo Benavides about.me/elbrujohalcon https://about.me/elbrujohalcon?promo=email_sig&utm_source=product&utm_medium=email_sig&utm_campaign=gmail_api&utm_content=thumb

Oh... wow... I was use more ugly approach 😄

{pre_hooks, [
    {compile, "erlc src/cowboy_swagger_hook.erl"},
    {compile, "erl -eval 'cowboy_swagger_hook:init()' -s init stop"}
]}.

elbrujohalcon · 2024-10-09T12:19:10Z

Please use an escript :)

vkatsuba · 2024-10-09T12:22:19Z

Please use an escript :)

The dark side clouds everything... but we remain one with the Force. 💫⚔️ May the light guide your way, even in Vader's shadow. Done!

vkatsuba · 2024-10-09T12:24:15Z

@elbrujohalcon, @paulo-ferraz-oliveira for now we use escript for create priv/swagger/swagger-initializer.js based on new priv/swagger/swagger-initializer.template. The swagger-initializer.js now is not a part of this repo and will be ignored

.github/workflows/up_swagger.sh

.gitignore

priv/update_swagger_url

rebar.config

elbrujohalcon · 2024-10-09T12:36:51Z

src/cowboy_swagger_redirect_handler.erl

+    SwaggerPath = application:get_env(cowboy_swagger, path, "/api-docs"),
+    {{true, SwaggerPath ++ "/index.html"}, Req, State}.


Ooooooh! So this is alredy generating issues in our production systems… nobody relized that because everybody just clicked a link that arleady contained the index.html part… HA! 🤦🏻

Now I see why you wanted to use path instead of the full url.
I don't think those 2 things should be necessarily related, @vkatsuba …
I would allow the user to configure the swagger base path (which I would call base_path or root_path, instead of path) on a parameter, and the swagger.json location (which I would call swagger_json_url or swagger_url) with another parameter.

Yes... make sense... So, now I suppose the code should be prepared as well for use at least path config. The application:get_env/3 should be able to get config from top project where it used. Still need to do end to end tests but we mostly there... So, we can prepare different variables now for use them in URL.

Ooooh… right… that's an issue. Have you tested your changes on another project using this one as a dependency?
I'm pretty sure it won't work as we expect… because the configuration for the cowboy_swagger app can be in whatever path… and thus it may not be visible to your new script.

I'll write an inline comment in the proper place…

Yes... end to end testing will be need to do...

elbrujohalcon · 2024-10-09T13:14:33Z

priv/update-swagger-url

+  case file:read_file(TemplateFilePath) of
+      {ok, Content} ->
+          %% Get the configuration for the path from the application environment
+          Path = application:get_env(cowboy_swagger, path, "/api-docs"),


This won't work. This script will run at compile-time in an erlang VM where cowboy_swagger is not yet compiled, and thus… not yet loaded. Therefore, no matter what the users of this library put on path in their sys.config files, it will be invisible to this script and therefore we'll end up with "/api-docs" everywhere. We need a different approach, one that acts at runtime instead of compile-time.

elbrujohalcon · 2024-10-09T13:19:12Z

@vkatsuba we need a very very different approach for this (as I mentioned in my inline comment). We need to…

Keep the template file in the repo.
Update the template file in up_swagger.sh, so that it has the placeholder.
Add a special trail to https://github.com/inaka/cowboy_swagger/blob/master/src/cowboy_swagger_handler.erl so that, when it handles a request for /api-docs/swagger-initializer.js, it reads the template, replaces the placeholder with the proper value and returns that… instead of returning the file directly.

elbrujohalcon · 2024-10-09T13:22:02Z

But @vkatsuba… please check cowboy_swagger_handler carefully… because, for instance, I had forgotten about the static_files env variable and that thing already allows users to pick up swagger files (jsons, htmls, everything) from a different path (instead of api-docs).

vkatsuba · 2024-10-09T13:22:14Z

@vkatsuba we need a very very different approach for this (as I mentioned in my inline comment). We need to…

1. Keep the template file in the repo.

2. Update the template file in `up_swagger.sh`, so that it has the placeholder.

3. Add a special trail to https://github.com/inaka/cowboy_swagger/blob/master/src/cowboy_swagger_handler.erl so that, when it handles a request for `/api-docs/swagger-initializer.js`, it reads the template, replaces the placeholder with the proper value and returns that… instead of returning the file directly.

But the points 1 and 2 already done?

paulo-ferraz-oliveira · 2024-10-12T21:24:52Z

.github/workflows/ci.yml

-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4


nit: This is Ok but shouldn't be required for this PR in particular, right?

paulo-ferraz-oliveira · 2024-10-12T21:26:20Z

.github/workflows/up_swagger.sh

@@ -22,8 +22,8 @@ rm -rf node_modules

 # Same as https://github.com/swagger-api/swagger-ui/blob/63ad6f6a5bce19075e717ea74acaf9f7055dcdf5/docker/docker-entrypoint.d/40-swagger-ui.sh#L12
 FIND="\"https://petstore.swagger.io/v2/swagger.json\""
-REPLACE="window.location.origin + \"/api-docs/swagger.json\""
-sed -i -e "s|${FIND}|${REPLACE}|g" priv/swagger/swagger-initializer.js
+REPLACE="window.location.origin + __SWAGGER_PATH__ + \'/swagger.json\'"


nit: Are the \" a problem here, that they had to be replaced with \'. If not, the change could be reverted?

paulo-ferraz-oliveira · 2024-10-12T21:27:16Z

priv/swagger/swagger-initializer.template

-    url: window.location.origin + "/api-docs/swagger.json",
+    url: window.location.origin + __SWAGGER_PATH__ + '/swagger.json',


nit: Same comment as before, for the ". Was it required to be changed?

paulo-ferraz-oliveira · 2024-10-12T21:28:37Z

README.md

+   %% `path`: Path where you can access Swagger-UI. Default: `/api-docs`
+   {path, "/api-docs-swagger"},


It says the default is /api-docs but we seem to be changing that. Do we need to change the default? I'm missing something, as I don't understand this particular change.

paulo-ferraz-oliveira · 2024-10-12T21:29:20Z

README.md

-      basePath => "/api-docs"
+      %% See "path" config section
+      basePath => "/api-docs-swagger"


Same here, I don't see why the base path (which is api-docs for all the projects I use this with) would be different from what it was. Can we restore it?

paulo-ferraz-oliveira · 2024-10-12T21:31:10Z

priv/update-swagger-url

@@ -0,0 +1,40 @@
+#!/usr/bin/env escript
+-module('update-swagger-url').


nit: I don't know about good practices for naming escripts, but I usually do it by replacing .erl with .escript, so this'd be update_swagger_url.escript, and then we could drop the enclosing quotes here (?)

vkatsuba force-pushed the issues-206 branch from 5bdf2a4 to 4068290 Compare October 9, 2024 07:54

vkatsuba marked this pull request as draft October 9, 2024 08:11

Add possibility to configure own path to "/api-docs/swagger.json"

cb4054f

vkatsuba force-pushed the issues-206 branch from 4068290 to cb4054f Compare October 9, 2024 09:04

vkatsuba added 2 commits October 9, 2024 12:12

Remove OTP 24 from CI due of errors build

588fc25

Some formatting of escript

37fa2f0

vkatsuba marked this pull request as ready for review October 9, 2024 09:27

Fix warnings in CI

50fbcf9

elbrujohalcon requested changes Oct 9, 2024

View reviewed changes

Add OTP 27 to CI matrix

8477453

vkatsuba force-pushed the issues-206 branch from 38389fb to 8477453 Compare October 9, 2024 11:08

Fix more warnings in CI

4848277

vkatsuba added 3 commits October 9, 2024 15:09

Stop ignoring swagger-initializer.js

32a1770

Change swagger-initializer.js to swagger-initializer.template

d9d20c5

Add to ignore swagger-initializer.js

357243d

Change hook approach

4ea4eaa

Formatting ...

a9b01ce

elbrujohalcon requested changes Oct 9, 2024

View reviewed changes

vkatsuba added 3 commits October 9, 2024 15:59

Rename hook to update-swagger-url

d3dd7c1

Start use *.template in up_swagger.sh

0e1e3a5

Remove cowboy_swagger_hook.beam from .gitignore

7d27098

Cleanup update-swagger-url escript

531d223

vkatsuba mentioned this pull request Oct 9, 2024

Some dependency or plugin require OTP 25 in CI where used OTP 24 #208

Open

elbrujohalcon reviewed Oct 9, 2024

View reviewed changes

Formatting of update-swagger-url

4a7ed61

vkatsuba force-pushed the issues-206 branch from 23b9652 to 4a7ed61 Compare October 9, 2024 13:15

vkatsuba marked this pull request as draft October 9, 2024 13:41

paulo-ferraz-oliveira reviewed Oct 12, 2024

View reviewed changes

	{project_plugins,
	[{rebar3_hank, "~> 1.4.0"},
	{rebar3_hex, "~> 7.0.7"},
	{rebar3_format, "~> 1.3.0"},
	{rebar3_lint, "~> 3.1.0"},
	{rebar3_ex_doc, "~> 0.2.20"}]}.

		@@ -0,0 +1,24 @@
		%% Reading the contents of the file
		case file:read_file("priv/swagger/swagger-initializer.js") of

		SwaggerPath = application:get_env(cowboy_swagger, path, "/api-docs"),
		{{true, SwaggerPath ++ "/index.html"}, Req, State}.

		url: window.location.origin + "/api-docs/swagger.json",
		url: window.location.origin + __SWAGGER_PATH__ + '/swagger.json',

		%% `path`: Path where you can access Swagger-UI. Default: `/api-docs`
		{path, "/api-docs-swagger"},

		@@ -0,0 +1,40 @@
		#!/usr/bin/env escript
		-module('update-swagger-url').

Add possibility to configure own path to "/api-docs/swagger.json" #207

Are you sure you want to change the base?

Add possibility to configure own path to "/api-docs/swagger.json" #207

Conversation

vkatsuba commented Oct 9, 2024 • edited Loading

vkatsuba commented Oct 9, 2024

elbrujohalcon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vkatsuba Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elbrujohalcon commented Oct 9, 2024 • edited Loading

vkatsuba commented Oct 9, 2024

vkatsuba commented Oct 9, 2024

elbrujohalcon commented Oct 9, 2024 via email

vkatsuba commented Oct 9, 2024

elbrujohalcon commented Oct 9, 2024 • edited Loading

vkatsuba commented Oct 9, 2024

vkatsuba commented Oct 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elbrujohalcon commented Oct 9, 2024

elbrujohalcon commented Oct 9, 2024

vkatsuba commented Oct 9, 2024

paulo-ferraz-oliveira Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

paulo-ferraz-oliveira Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

paulo-ferraz-oliveira Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulo-ferraz-oliveira Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

vkatsuba commented Oct 9, 2024 •

edited

Loading

elbrujohalcon left a comment •

edited

Loading

vkatsuba Oct 9, 2024 •

edited

Loading

elbrujohalcon commented Oct 9, 2024 •

edited

Loading

elbrujohalcon commented Oct 9, 2024 •

edited

Loading

paulo-ferraz-oliveira Oct 12, 2024 •

edited

Loading

paulo-ferraz-oliveira Oct 12, 2024 •

edited

Loading

paulo-ferraz-oliveira Oct 12, 2024 •

edited

Loading

paulo-ferraz-oliveira Oct 12, 2024 •

edited

Loading