rust-lang / triagebot

Automation/tooling for Rust spaces
https://triage.rust-lang.org
Apache License 2.0
170 stars 74 forks source link

Automate documentation updates for rust-lang/rust #1673

Closed ehuss closed 1 year ago

ehuss commented 1 year ago

This adds a scheduled job that will periodically create a PR that updates the documentation submodules on rust-lang/rust. This was previously handled by me manually running a separate tool, and I wanted to set up a cron job to further automate the process.

ehuss commented 1 year ago

This will require creating a branch named "docs-update" on rust-lang/rust. This also presumes that the GITHUB_TOKEN that triagebot runs with has enough permissions to write to that branch.

Mark-Simulacrum commented 1 year ago

This looks pretty good (just skimmed it, will do a full review later). I'm curious whether it seems viable to move to branch to the fork rustbot already (?) has, that seems like it's hopefully no difference in the code but avoids any pollution of the branches in the main repo and permissions discussion.

I'd also love feedback since I know I pushed for this approach - did this seems lightweight compared to GitHub actions? How did the cron support work out (do we need to change the API there or whatever)?

ehuss commented 1 year ago

I wasn't aware that rustbot has a fork. Which repo is that? I think it should be relatively easy to point it there.

This was quite a lot more work than using GitHub Actions, only because I needed to rewrite subup to use the GitHub API instead of using git commands directly. The cron support in triagebot was the trivial part and in my testing seemed to work fine.

The hardest part was that I had to learn GraphQL. Every time I've tried using it in the past, I gave up as it seemed too complicated. However, I really needed something like git log --first-parent rev1..rev2, and GitHub doesn't offer anything remotely close to that in REST or GraphQL (but the GraphQL API is much better than the REST one). On the plus side, this now relies on GitHub's "associated_pull_requests" for determining which PRs are part of the change instead of using regex to scrape the log messages, which was unreliable.

Mark-Simulacrum commented 1 year ago

Oh, sorry. I think we didn't have a fork before but I've now created one (https://github.com/rustbot/rust), that should have write access etc by rustbot pretty easily.

Yeah, I hear you on this being more work up front. Hopefully the one-time upfront cost is justified by the long-term benefits I think we get from it, but we'll have to see. Nice to have the improved PR discovery implemented.

ehuss commented 1 year ago

Yea, I'm glad to finally get a chance to dig into GraphQL. It's not so bad once you figure out the basics.

I pushed an update that will use rustbot/rust to create the commits.

ehuss commented 1 year ago

Can you also create a docs-update branch on rustbot/rust? This won't work without it already existing.

apiraino commented 1 year ago

Yea, I'm glad to finally get a chance to dig into GraphQL. It's not so bad once you figure out the basics.

One thing that I find confusing is how to get the GraphQL query from Rust source code. I can easily generate the Rust boilerplate using cynic code generator but how to do the opposite? What if I want to tweak a current GraphQL query and test it (which is what am I doing to the other GraphQL query in this codebase)?

I'm tempted to think that it would be useful to attach somewhere (as rustdoc comment?) a reproducible GraphQL represention of each query in github-graphql/src/lib.rs

Opinions?

ehuss commented 1 year ago

That's a good question. Calling build() on the query will give you JSON that you can print which contains a string of the query. That's not terribly convenient to access, and requires passing in some dummy variables for the arguments. A doc comment sounds like a good idea as long as there is some discipline to keep it in-sync. Maybe open an issue with cynic to see if there are some other ideas? Maybe a doctest or proc-macro or something could help with ensuring it is in sync.

Posted #1675 which includes the query I used here.

apiraino commented 1 year ago

@ehuss thanks for the additional patch, very convenient. Also, I didn't know about the .build(). Could come in handy if we want to add tests when we update the Rust code (ex. upgrading to cynic 2.x - we have the guarantee that the same GraphQL query will be generated).

Mark-Simulacrum commented 1 year ago

Can you also create a docs-update branch on rustbot/rust? This won't work without it already existing.

I will do this later today.

Mark-Simulacrum commented 1 year ago

Created.