[Chore] Add swag fmt to mage spec

[andresuribe87]

No description provided.

[codecov-commenter]

Codecov Report

Merging #198 (237077b) into main (8669899) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #198   +/-   ##
=======================================
  Coverage   24.99%   24.99%           
=======================================
  Files          31       31           
  Lines        2969     2969           
=======================================
  Hits          742      742           
  Misses       2105     2105           
  Partials      122      122           

Impacted Files	Coverage Δ
pkg/server/router/presentation.go	3.65% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[decentralgabe]

@andresuribe87 to try not forking and see if it works

[michaelneale]

#239 shows it can work

[michaelneale]

@andresuribe87 what are the problems with this?

[decentralgabe]

@michaelneale the problem is that all of our community contributions are from forks

[michaelneale]

@decentralgabe @andresuribe87 right but only really want swagger updated when things are merged or when there is a release perhaps?

or it can publish the swagger to somewhere else?

[decentralgabe]

publishing to our website would be good, but we need a source of truth...which should be this repo. my gut says we should keep it up to date for every commit. I would expect main to have the current accurate state

[michaelneale]

ok so to sum up:

0. swagger docs are magically generated as part of the build
1. want docs committed to the repo as the source of truth on main/relevant branch
2. most contributions happen via fork, and it seems the token can't push back to that fork (https://github.com/orgs/community/discussions/25702)
3. even if you do it on a branch, it kind of ruins the flow as after you push then it commits a subsequent change which means the the head of the branch doesn't have the green tick of the action on it (but the previous commit does)
4. generation of swagger docs is not deterministic see Multiple runs generate different swagger files / Flaky generation swaggo/swag#721

So due to #4 we can't check that swagger docs are up to date as part of build (but if we could, then the docs could be generated as part of pre-commit task from developer).

Number 3 is due to https://github.com/orgs/community/discussions/25702 - which seems by design. A work around is to use a PAT (this would probably fix #2 as well - but it is not ideal from a security point of view)

Another idea: have it only trigger on merge to main, but then have it push to a docs branch (which gets around #3, and #2)
so you push some code, and then you get main_docs branch with the docs

Another another idea: something like https://github.com/marketplace/actions/swagger-ui-action - which means it publishes it to github pages

Another even different idea: have it open a pull request instead of just pushing to main, but have it check if the docs have changed (I like this one) but it means making sure it is idempotent (I don't see why it shouldn't be - that is probably main problem to solve).

Does any of the above help @andresuribe87?

[michaelneale]

@decentralgabe @andresuribe87 played around a bit - I think this will need to be pulled out into its own workflow (see review comment).

Also - if the swagger docs are purely generated from the swag command and the source - do they need to be committed to the repo? Could they instead be published to a github pages branch? (just a thought).

I would be curious what you aim for by having the api spec in the repo as generated - I guess one advantage would be that you can see a clear diff over time, or if a PR changes an api (breaks). If this is the aim - I would do this a different way with a PR specific workflow which checks if the spec is different and asks/updates it for them.

I think in any case you don't really want to run this on main as well do we? (correct me if wrong). To make this work properly it looks like both a separate workflow is needed as well as a personal access token. I tried to run mage locally but something about install for go was broken for me, so couldn't try locally.

[michaelneale]

this workflow runs not just on PRs but on merges to main - so this would fail at that point as there is no pull_request.head.

I think this should be pulled out into its own workflow for swagger publish.

[andresuribe87]

Does any of the above help @andresuribe87?

Yes, that's an excellent summary. Personally I think we might just want to fix upstream; but we may run into other issues when upgrading the deps.

Yet another idea is to rename structs used for http (e.g. router.SubmitApplicationRequest => router.SubmitApplicationHTTPRequest). I have a hunch that swaggo is getting confused with repeated package names, plus the fact that we're parsing deps.

do they need to be committed to the repo? Could they instead be published to a github pages branch?

That's a very valid point. I personally don't have a preference.

I think in any case you don't really want to run this on main as well do we?

I think it depends on the the what. For example, I can see value in having an action that publishes the generated docs on every commit to main. On the other hand, if we're pushing the generated commits, then that should be on every PR, but not on main.

[michaelneale]

I am closing this in favour of: #249