Skip to content
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

Add github action to generate and push docs to docs branch #695

Closed
wants to merge 3 commits into from

Conversation

dfreese
Copy link
Collaborator

@dfreese dfreese commented Apr 19, 2021

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.

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.
@dfreese dfreese marked this pull request as ready for review April 19, 2021 16:27
@UebelAndre
Copy link
Collaborator

I think it'd be better to keep updated docs on main (or whatever branch contains the change). I definitely refer to docs which live in source code rather than github pages since the docs are not versioned on github pages.

@dfreese
Copy link
Collaborator Author

dfreese commented Apr 19, 2021

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.

@dfreese dfreese requested a review from hlopko April 19, 2021 16:39
@@ -78,14 +78,6 @@ tasks:
working_directory: examples
test_targets:
- //...
docs_linux:
Copy link
Collaborator

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

Copy link
Collaborator

@illicitonion illicitonion left a 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

.github/workflows/gen_docs.yaml Outdated Show resolved Hide resolved
@UebelAndre
Copy link
Collaborator

I prefer to keep auto-generated code out of the repository, I've found it adds noise to reviews.

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.

There's the actual docs attributes that can be referenced, and this removes the duplication.

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.

@UebelAndre
Copy link
Collaborator

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 <dawagner@gmail.com>
@dfreese
Copy link
Collaborator Author

dfreese commented Apr 20, 2021

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.

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. docs/X.Y.Z, put up as an archive in the release (as you suggested), or layered in a single archive for all versions.

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 flatten.md document add a significant amount of noise to any commit which updates the documentation.

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:

  1. Markdown available on github (on the docs branch)
  2. The github page
  3. Being automatically generated

@dfreese dfreese requested a review from UebelAndre April 20, 2021 21:29
@UebelAndre
Copy link
Collaborator

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.

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.

Copy link
Collaborator

@UebelAndre UebelAndre left a 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).

Copy link
Collaborator

@UebelAndre UebelAndre left a 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this still used?

Copy link
Collaborator

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
Copy link
Collaborator

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

Copy link
Member

@hlopko hlopko left a 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.

@hlopko
Copy link
Member

hlopko commented May 12, 2021

I'd like to change the phrasing of my last comment. I'm impatiently excitedly looking forward to having this PR merged :)

@alexeagle
Copy link
Contributor

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 🌿

@UebelAndre
Copy link
Collaborator

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!

@hlopko
Copy link
Member

hlopko commented May 19, 2021

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.

@alexeagle
Copy link
Contributor

thanks @hlopko - there's nothing you need to do to integrate though. Just have stardoc rules and .md files in this repo.

@hlopko
Copy link
Member

hlopko commented May 20, 2021

@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).

@hlopko
Copy link
Member

hlopko commented Jul 1, 2021

@dfreese do you plan to continue working on this PR?

@hlopko
Copy link
Member

hlopko commented Jul 1, 2021

Or is https://docs.aspect.dev/ going to be the place where we host docs for rules rust?

@UebelAndre
Copy link
Collaborator

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: bazelbuild/rules_foreign_cc#690

I setup https://bazelbuild.github.io/rules_foreign_cc/ in response. Just FYI

@alexeagle
Copy link
Contributor

@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

  • the Jekyll templating engine is hard to run under Bazel, and transforming Markdown to whatever that thing needs is an endless headache. We had to maintain our own stardoc velocity templates too. When you're coding in multiple templating engines at once, you get sad
  • GH pages doesn't support versioning so when we make breaking changes it's annoying to show a nice docsite for the earlier release
  • things like search, rendering permalinks were an extra chore

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!

@dfreese dfreese closed this Jan 4, 2023
@dfreese dfreese deleted the to_docs branch January 4, 2023 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants