-
-
Notifications
You must be signed in to change notification settings - Fork 24
Fix: docs CI workflow should only publish when on master #310
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
Fix: docs CI workflow should only publish when on master #310
Conversation
joshbax189
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some notes for checking
.github/workflows/docs.yml
Outdated
| run: | | ||
| npm install | ||
| npm run build | ||
| run: curl -L https://github.com/thegeeklab/hugo-geekdoc/releases/latest/download/hugo-geekdoc.tar.gz | tar -xz --strip-components=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more of a suggestion. Per the geekdoc docs, since you are just using the standard theme, I think you can save most of the time in this step by just using the pre-built tarball here.
If you like it (and it works) I can remove the submodule for geekdocs too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
publish-doc
succeeded 8 minutes ago in 10s
Oh yeah!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked it locally by:
- running the
curlcommand indocs/themes/geekdocs - starting hugo using a container (run in the
docsdirectory):docker run --network=host --mount type=bind,source=.,destination=/mine/ -w /mine peaceiris/hugo:v0.134.2 server - visit the ip address it suggests
Looks like the theme is loaded correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the suggestion! I appreciate it, but I prefer using the old method as it’s more reproducible for me. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked it locally
BTW this was just my way of avoiding installing hugo, and guaranteeing the versions matched upstream.
For local development, you could absolutely keep the submodule method. It would match this one unless you switched to a different branch on the hugo-geekdoc repo.
Could also put the curl command into a Make rule that matches what happens in this file, so (assuming you have hugo locally), you just do make docs: https://geekdocs.de/usage/getting-started/#use-a-makefile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW this was just my way of avoiding installing hugo, and guaranteeing the versions matched upstream.
For local development, you could absolutely keep the submodule method. It would match this one unless you switched to a different branch on the hugo-geekdoc repo.
I see the YAML file as more of a reference for setting up the environment. I also want to eliminate other potential variables to improve the debugging experience. While this approach may increase CI runtime, it’s a trade-off that will save me time in the long run. 🤔
I guess I'm being a bit selfish here... but I might change my mind in the future. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries. Can always refer back to this! Will drop this commit
|
Hmm.. maybe all the other CI workflows should ignore some changes too? |
You're absolutely right! This has been on my to-do list, but I never got around to it. 😅 |
ac388fb to
7a7ab89
Compare
|
LGTM! Thank you so much for your hard work! 🥳 🚀 |
Suggested change in other PR. See review comments on code.