Skip to content
This repository has been archived by the owner on Dec 12, 2024. It is now read-only.

[OSE-311] Docs publish #239

Closed
wants to merge 25 commits into from
Closed

[OSE-311] Docs publish #239

wants to merge 25 commits into from

Conversation

michaelneale
Copy link
Contributor

@michaelneale michaelneale commented Jan 6, 2023

Trying this from a branch - same as #198

@codecov-commenter
Copy link

codecov-commenter commented Jan 6, 2023

Codecov Report

Merging #239 (2f8132c) into main (960a062) will not change coverage.
The diff coverage is n/a.

❗ Current head 2f8132c differs from pull request most recent head a7f6874. Consider uploading reports for the commit a7f6874 to get more accurate results

@@           Coverage Diff           @@
##             main     #239   +/-   ##
=======================================
  Coverage   25.17%   25.17%           
=======================================
  Files          31       31           
  Lines        3023     3023           
=======================================
  Hits          761      761           
  Misses       2145     2145           
  Partials      117      117           
Impacted Files Coverage Δ
pkg/server/router/credential.go 3.57% <ø> (ø)
pkg/server/router/did.go 7.92% <ø> (ø)
pkg/server/router/health.go 0.00% <ø> (ø)
pkg/server/router/issuance.go 0.00% <ø> (ø)
pkg/server/router/keystore.go 12.69% <ø> (ø)
pkg/server/router/manifest.go 9.81% <ø> (ø)
pkg/server/router/operation.go 0.00% <ø> (ø)
pkg/server/router/presentation.go 3.65% <ø> (ø)
pkg/server/router/readiness.go 0.00% <ø> (ø)
pkg/server/router/schema.go 9.19% <ø> (ø)

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

Copy link
Contributor

@andresuribe87 andresuribe87 left a comment

Choose a reason for hiding this comment

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

Thank you!

@andresuribe87
Copy link
Contributor

A couple of issues that need to be addressed before merging this PR:

  • Automatic commits do not trigger github actions. More details about this in this README, and potential solutions (like creating a robot account with a personal access token).
  • Code generation is not idempotent (try running mage spec, then git commit -a -m "commiting spec", then mage spec; you'll notice that there is still a diff when you run git diff --shortstat). This would cause and inifinite recursion of github actions if we were to trigger actions on automatic commits.

@andresuribe87 andresuribe87 changed the title Docs publish [OSE-311] Docs publish Jan 6, 2023
@michaelneale
Copy link
Contributor Author

@andresuribe87 yeah I guess it’s good it doesn’t trigger actions though right? (That must be by design?). But the statuses end up hanging. I wonder if there is a way to make things green when it’s from the action bot.

@michaelneale
Copy link
Contributor Author

Maybe can do it just on merge to main? Not on each PR?

@michaelneale
Copy link
Contributor Author

hrm - the conflict will be annoying.

I wonder if there is a better way to publish docs - should they be stored in the repo? (I guess so).

@decentralgabe
Copy link
Member

Maybe can do it just on merge to main? Not on each PR?

it should be done on each commit to avoid it getting out of sync

@michaelneale
Copy link
Contributor Author

@andresuribe87 you want to merge this one or the other one?

@andresuribe87
Copy link
Contributor

@andresuribe87 you want to merge this one or the other one?

Unfortunately, we cannot merge this yet until we figure out the issues mentioned. I'll be digging into it further this week.

@decentralgabe
Copy link
Member

a dumb approach, that might just work is to have a check that fails if the swagger is not up to date. this forces the user to regen and re-commit and we don't need to worry bout the automation headache

@andresuribe87
Copy link
Contributor

Note that swaggo currently is not idempotent. In particular, the approach of generating the file and detecting a diff to see if swagger is up to date would not work since as the result will always yield a diff.

We could do a hack where we measure the diff size. I'm not sure if that's stable though.

@decentralgabe
Copy link
Member

how do we make it idempotent?

@michaelneale
Copy link
Contributor Author

closing as this was just to try something out.

@michaelneale michaelneale deleted the docs-publish branch January 11, 2023 05:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants