-
Notifications
You must be signed in to change notification settings - Fork 474
Add github action to generate and push docs to docs branch #695
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
Conversation
Instead of requiring that users regenerate auto-generated documentation each time it changes, this adds a github action to force push any changes on the main branch to the docs branch. Github pages allows docs to be on any branch, in the docs folder. There may be some permissions issues with the docs branch. In that case, I'm fine reverting this immediately afterwards. I've tested this to the extent I could on my forked repository.
I think it'd be better to keep updated docs on |
I prefer to keep auto-generated code out of the repository, I've found it adds noise to reviews. There's the actual docs attributes that can be referenced, and this removes the duplication. |
.bazelci/presubmit.yml
Outdated
@@ -78,14 +78,6 @@ tasks: | |||
working_directory: examples | |||
test_targets: | |||
- //... | |||
docs_linux: |
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.
Docs should still be built in CI to ensure there are no issues generating them
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.
done.
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 find docs updates very noisy in reviews, but I don't have a super strong opinion either way
I understand the benefit of treating docs as an artifact but I don't think it's sufficient to throw them in a separate branch. This branch should first, be protected (only the bot or PRs can make changes there) and it should be crystal clear what rev of docs matches what rev of the code.
Linking to cleanly rendered docs is preferred to linking to docstrings. I'd like to maintain this behavior so sharing docs isn't unnecessarily difficult. |
This said though, I think using releases to version the docs "artifacts" would be sufficient for my use cases. I can imagine many ways where we could clearly align a rev of docs with a particular release. But I don't think it'd be good to make this change without having that story in place (for how to associate a rev of docs with a rev of the rules). |
Co-authored-by: Daniel Wagner-Hall <[email protected]>
Only maintainers can make changes to branches in the repository, so I think that is sufficiently protected. As of right now, this only represents main. I think there are enough reasonable avenues where additional docs branches could be created (e.g. I don't think this makes sharing docs more difficult for the current main version. The rendered markdown would be available for the latest main version, just on the docs branch. This does however, solve a problem from my end, which is that diffs caused by the auto-generated documentation, particularly the Given that any sort of docs page will probably want to be generated for all releases (or the most recent ones, for example), not just any single commit, this felt like a reasonable compromise of clarifying individual diffs, while still preserving the core functionality:
|
Docs at main are not the issue, It's docs at a particular commit. This affects my workflow and confidence in the accuracy of the documentation given a particular rev, but I'm happy to have releases solve for this. I can start a draft for one but would ask for help in adding notable changes to the release notes. If there's anything else that you think we should do for a release, we could discuss on #415. |
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.
Approving so I'm not rudely blocking you, but really do want some help in getting a release out (given that the last one was years ago).
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.
A few things I noticed which might clean this up a bit
@@ -1,6 +1,6 @@ | |||
#!/bin/bash |
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.
Is this still used?
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.
If not, it should probably be deleted and removed from the Docs job in BuildKite
runs-on: ubuntu-20.04 | ||
steps: | ||
- uses: actions/checkout@v2 | ||
- name: Install bazelisk |
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.
Also, seems Bazelisk is available on Ubuntu-2004 machines: https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-README.md
This step could probably be removed
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.
LGTM. I support this PR. For PR reviews the generated files IMHO do add noise. I often times forget to regenerate docs, then buildkite fails, then I fix that. So automation in this PR is a quality of life improvement for me. I will say that now we at least have the buildkite notifying me that I forgot, it was worse before :)
I actually use the generated pages a lot, I send links to colleagues, and I also go there personally. For our use cases we don't need versioned docs urgently.
I'd like to change the phrasing of my last comment. I'm impatiently excitedly looking forward to having this PR merged :) |
I think the rules ecosystem would benefit from more collaboration. Right now it feels like lots of small understaffed maintenance teams having to reinvent things. I'd like to propose a rules docsite (Googlers think fondly of g3doc) that just does the right thing. You should only need stardoc rules. TAL at https://docs.aspect.dev where I've started this already 🌿 |
I love the site! I'd love to help make that a more structured thing and standardize how we (and other rules) use it for publishing docs. The next thing I need is to know where to open a feature request for a dark theme 😆 But seriously, awesome stuff Alex, thanks a lot! |
The site indeed looks really good, thank you Alex. I believe there are discussions with the Bazel team about details, but in the meantime I'm told we can go ahead and integrate rules rust with the website. Let's do that in a separate PR though. |
thanks @hlopko - there's nothing you need to do to integrate though. Just have stardoc rules and .md files in this repo. |
@alexeagle are you building those stardoc rules on your end? We already have stardoc rules in this repo, and we manually regenerate the website on each commit. This PR changes that to be an automated process behind the scenes. If docs.aspect.dev did the build (cd docs && bazel build //...) that would save us a ton of trouble (assuming other maintainers would be fine with not having https://bazelbuild.github.io/rules_rust/, but that'd be a followup discussion). |
@dfreese do you plan to continue working on this PR? |
Or is https://docs.aspect.dev/ going to be the place where we host docs for rules rust? |
There was recently concerns raised about the use of a 3rd party site for docs: bazel-contrib/rules_foreign_cc#690 I setup https://bazelbuild.github.io/rules_foreign_cc/ in response. Just FYI |
@hlopko I was helping with this in rules_foreign_cc and will repeat here: docs.aspect.dev is indeed a third-party site. I'm funding this on my own dime because I think it's really useful for the bazel community, and as an author of rules_nodejs we spent a ton of time building out our GitHub-pages docsite only to discover it could never be great
So, my intent is that rules authors shouldn't need to deal with the pretty docsite on GH pages anymore. It is hard, and not economical for every rules maintainer to make their own local solution to an ecosystem-wide problem. OTOH I think you have an obligation to your users to provide a self-contained repo. They shouldn't be forced to use a third-party site, and also if they clone or vendor your repo it should have readable docs. I think your obligation is 100% met by just ensuring that the rendered markdown is checked into the repo. GitHub renders it fine, and you can always find the docs for a particular version by just navigating to that version in the GH UI. So, my recommendation is to do like rules_apple, rules_swift, and others. Have a test-plus-update-command that is convenient for contributors (see bazelbuild/rules_apple#1194 ) and then don't bother with anything else related to rendering documentation. Happy to chat on bazel slack if you'd like! |
Instead of requiring that users regenerate auto-generated documentation
each time it changes, this adds a github action to force push any
changes on the main branch to the docs branch. Github pages allows
docs to be on any branch, in the docs folder.
There may be some permissions issues with the docs branch. In that
case, I'm fine reverting this immediately afterwards. I've tested this
to the extent I could on my forked repository.