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

[Merged by Bors] - migration guide and release note generator #469

Closed
wants to merge 28 commits into from

Conversation

IceSentry
Copy link
Contributor

@IceSentry IceSentry commented Oct 25, 2022

Objective

Generate migration guide and release note automatically.

Solution

The general steps are pretty simple.

  • Get all the commits on main
  • Get all PRs merged by bors and make sure they were closed after the target date
    • For the migration guide, make sure the PR has the C-Breaking-Change label
  • For each commit, find the corresponding PR by using the title of the PR
    • We do this because github doesn't return a commit hash with the PR and some of the metadata we want needs a commit hash
  • For each commit, get the list of contributors from the graphql api. For whatever reason, github doesn't give this information through the rest api
    • This is pretty slow because it requires a network call for each commit unfortunately. There's probably some ways to speed this up, but it does the job and takes about 3 minutes on my machine. To avoid being rate limited, I wait for a few seconds and then retry when it fails and it seems to work well.

Notes

This doesn't necessarily need to live in the repo, but since I have the code and we'll probably reuse it in the future, might as well share it.

The markdown generation is a bit intense because the parser I used wouldn't let me simply write it back, so I needed to implement it myself.

@rparrett
Copy link
Contributor

I think the parsing could likely be simplified with into_offset_iter.

@IceSentry
Copy link
Contributor Author

I'm not sure how I could use it to simplify the parsing.

@rparrett
Copy link
Contributor

rparrett commented Oct 25, 2022

Rather than re-rendering the markdown, you could use use the returned range to grab the source markdown for the content following the header.

Although you're getting some value (e.g. the code block language stuff) out of doing it this way.

@IceSentry
Copy link
Contributor Author

Yeah, I just tried it, the code is a bit simpler, but the output generates a lot more errors when going through markdownlint. Since I already have the code I'll just leave it as is.

@IceSentry IceSentry changed the title migration guide generator migration guide and release note generator Oct 25, 2022
@cart
Copy link
Member

cart commented Nov 7, 2022

As a heads up, when generating the list of prs for 0.9, this PR shows up: bevyengine/bevy#3223.
Notably, I deleted the ignore-reddit branch 7 days ago, which probably affected the metadata being used. This seems to imply that we might need to use a different date/time data source.

@cart
Copy link
Member

cart commented Nov 7, 2022

(there are quite a number of these actually. just look at some of the lower numbered prs)

@IceSentry
Copy link
Contributor Author

Oh yeah, I never filtered on the branch, didn't think about that. I used the issues api because PRs are issues and it worked, but there's actually a pulls api that let's me filter on the branch https://docs.github.com/en/rest/pulls/pulls I'll try to update it later tonight to use it instead.

@cart
Copy link
Member

cart commented Nov 7, 2022

Nice nice! Short term I've come up with a solution:

  • Add GithubIssuesResponse::closed_at
  • Extract the date from it
  • Filter out anything before the given date

Very easy tweaks to the existing infra! So I'm unblocked for this release. But using the branch is a more exact solution, so we should definitely use that if we can.

@rparrett
Copy link
Contributor

rparrett commented Nov 7, 2022

Another note: Need to manually filter out PRs that were cherry-picked for 0.8.1

This should be all of them.

(Or not, since this is technically "new stuff since the last blog post")

- [bevy_pbr: Fix tangent and normal normalization][5666]
- [Sync up bevy_sprite and bevy_ui shader View struct][5531]
- [fix order of exit/close window systems][5558]
- [Fix View by adding missing fields present in ViewUniform][5512]

@cart
Copy link
Member

cart commented Nov 7, 2022

(Or not, since this is technically "new stuff since the last blog post")

Yeah I'm cool with including these (at least for the contributors list) to properly credit contributors.

bors bot pushed a commit that referenced this pull request Nov 12, 2022
Migration guide generated by #469 

This is mostly just the generated output with a tiny bit of manual modifications.

Co-authored-by: Charles <IceSentry@users.noreply.github.com>
Co-authored-by: Rob Parrett <robparrett@gmail.com>
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

IMO we should scrape commits on main, like you said, and then extract emails from there. Once that's done this LGTM.

@cart
Copy link
Member

cart commented Feb 15, 2023

I'm still seeing commits from previous releases in the outputs (and like the previous issue mentioned above, it seems like its because its using "last modified" instead of "closed at").

@cart
Copy link
Member

cart commented Feb 15, 2023

Likewise, there are a couple of stragglers on the "boundary" of 2022-11-12 because the date on its own doesn't match the exact time of the 0.9 release. If possible, if we can take a "commit range" as input (or preferably branch names like v0.9.0..origin/main) that would allow us to be perfect and only include the exact commits we need.

@cart
Copy link
Member

cart commented Feb 15, 2023

@IceSentry
Copy link
Contributor Author

it seems like its because its using "last modified" instead of "closed at"

🤔 That's a bit strange, because it is supposed to be using the closed_at field. So it might be that github named it differently in the api.

there are a couple of stragglers on the "boundary" of 2022-11-12

I completely forgot about that. I originally used a date because it was easier to figure out and less manipulation for me, but it's pretty much just used to retrieve the oldest commit on that date and everything derives from it. I'll just take in the actual commit sha instead. Maybe I could use a git tag, assuming we even have those for each version, but a commit sha should do the trick.

@cart
Copy link
Member

cart commented Feb 15, 2023

This api appears to do the trick:

    pub fn compare_commits(&self, from: &str, to: &str) -> anyhow::Result<()> {
        let request = self.get(&format!(
            "https://api.github.com/repos/bevyengine/bevy/compare/{from}...{to}"
        ));
        let response = request.call()?;
        let str = response.into_string().unwrap();
        println!("{str:#?}");
        Ok(())
    }

@cart
Copy link
Member

cart commented Feb 15, 2023

Where from is "v0.9.0" and to is "main"

@cart
Copy link
Member

cart commented Feb 15, 2023

Just returns a giant blob of json with every commit + some metadata. Only contains the first committer, so we'll still need to make calls to the other apis to get those.

Haven't plugged this in to anything yet / I've gotta hop out for a bit. So feel free to do that if you're interested.

@IceSentry
Copy link
Contributor Author

I need to go out for a bit too, but I'll do it later tonight.

@cart
Copy link
Member

cart commented Feb 15, 2023

I found some time to get this working. Thankfully I could pretty much directly plug this in to what you wrote. Need to head out again, but its all wired up.

@cart
Copy link
Member

cart commented Feb 15, 2023

Pushed my work so far. The number of "matching" PRs isn't right (~250 instead of the ~430 after the base commit date). Maybe the "title matching" algorithm needs changing? I've got to run again :)

@cart
Copy link
Member

cart commented Feb 15, 2023

Well that would explain it ... the compare api is only returning 250 commits. Maybe we need paging or something?

@IceSentry
Copy link
Contributor Author

Ah right, yeah, I had to support paging for the other api I used. Should be easy enough to reuse it. If you haven't done it yet I can do it in an hour.

@IceSentry
Copy link
Contributor Author

@cart I implemented the paging. Right now it finds 466 commits using from v0.9.0 to main. I haven't checked if this is all correct, but it's more than 250 lol

I also removed some dead code so the diff is a bit noisy

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Great work on this. Thanks for making our lives much easier!

@cart
Copy link
Member

cart commented Feb 17, 2023

bors r+

bors bot pushed a commit that referenced this pull request Feb 17, 2023
## Objective

Generate migration guide and release note automatically.

## Solution

The general steps are pretty simple. 

- Get all the commits on main 
- Get all PRs merged by bors and make sure they were closed after the target date
	- For the migration guide, make sure the PR has the `C-Breaking-Change` label
- For each commit, find the corresponding PR by using the title of the PR
	- We do this because github doesn't return a commit hash with the PR and some of the metadata we want needs a commit hash
- For each commit, get the list of contributors from the graphql api. For whatever reason, github doesn't give this information through the rest api
	- This is pretty slow because it requires a network call for each commit unfortunately. There's probably some ways to speed this up, but it does the job and takes about 3 minutes on my machine. To avoid being rate limited, I wait for a few seconds and then retry when it fails and it seems to work well.

## Notes

This doesn't necessarily need to live in the repo, but since I have the code and we'll probably reuse it in the future, might as well share it.

The markdown generation is a bit intense because the parser I used wouldn't let me simply write it back, so I needed to implement it myself.


Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@bors
Copy link

bors bot commented Feb 17, 2023

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title migration guide and release note generator [Merged by Bors] - migration guide and release note generator Feb 17, 2023
@bors bors bot closed this Feb 17, 2023
bors bot pushed a commit that referenced this pull request Mar 5, 2023
First draft of the 0.10 migration guide.

This is generated by #469 and then modified manually

Since this is easy to generate, I will re-generate it multiple time before release so changing the contents of guides is encouraged, but changing ordering should be done closer to release to avoid dealing with multiple conflicts.

For this migration guide, I added tags for areas of each PR, added a bit of spacing and a line for each heading
![image](https://user-images.githubusercontent.com/8348954/216899165-6f99a25f-e32d-42a4-bad4-8e23c7aac366.png)

This is why there are some css files in this pr.


Co-authored-by: IceSentry <IceSentry@users.noreply.github.com>
Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants