Proposal: API Reviews via GitHub

[tjprescott]

This proposal describes a system for conducting API reviews using GitHub rather than APIView.

A POC for various scenarios can be found here:
Azure/azure-sdk-for-python#47203

[lmazuel]

I've been thinking about this one. If a service team does PR from one version to the next, I guess the diff is the exact diff between released version.

Now if they do feature by feature, each diff would be more incremental. I guess it's still ok, because each increment are still valid diff, if they don't show the full picture.

Just thinking out loud, I think I'm good if diff are incremental, which would be the simplest likely here.

[tjprescott]

It's really going to come down to when you choose to open an API Review PR. If you had PRs for features A, B and C that are all going into version X, it would make sense to wait to produce the API review PR until all three features are merged.

[m-nash]

If I understand correctly ApiView today lets you approve the changes for A, B, and C independently and as long as no new changes come in your release would be unblocked since its able to understand that combined there are no new API changes.

With the new system I would need to review A, B, and C and then again review X (the combined version) and everything in X would appear "new" even though it was previously reviewed?

I am not sure waiting for X is feasible since the doc mentions architects will be set as CodeOwners of the api.md file and it also said that the api.md file will change with every pr (that touches public api surface) enforced by the ci. So the architect will be pinged on every one of those to review.

Also waiting to review it all as one big change gets noisy especially if the features are not related. Reviewing in smaller chunks is always preferred.

[tjprescott]

If I understand correctly ApiView today lets you approve the changes for A, B, and C independently and as long as no new changes come in your release would be unblocked since its able to understand that combined there are no new API changes.

So that's not the case today. Your release is ungated if the release artifact's API hash matches any approved revision. In the case where you approve A, B, and C individually, the combined API would have a different hash, so you would still need to (at some point) approve the combined API for the release to be ungated.

You would still have the ability to review features, probably targeting PRs. Most likely, when the service team needs the final approval, they would link to those approved PR reviews. In that sense it would be the same as today--it's just the process of creating the reviews (and seeing that they are approved) is less obvious since they aren't grouped.

[m-nash]

Gotcha thanks for the clarification.

Most likely, when the service team needs the final approval, they would link to those approved PR reviews

Can this be discovered / done automatically as part of the tooling that creates the review PR?

[heaths]

Long-lived and/or disparate features should probably be done in feature branches anyway, but then that begs the question I asked above: how does that work with any gates to release from a feature (or release) branch?

[tjprescott]

A release gate will compare the API hash of the release artifact to an "approved" release hash that comes from approving a review PR. That approval will be stored on the ADO work item for the package in question. So it really doesn't matter whether the approval was on a release branch, feature branch, etc or what the origin of the release artifact is.

[maorleger]

I am not sure waiting for X is feasible since the doc mentions architects will be set as CodeOwners of the api.md file and it also said that the api.md file will change with every pr (that touches public api surface) enforced by the ci. So the architect will be pinged on every one of those to review.

That's really not sustainable for us architects (assuming I understood the flow correctly). This used to be the case for JS (where we already generate api.md files under a review folder) but we were unable to keep up with the flood of api.md changes. A way to "batch" changes to a given review is really nice. Maybe that's something that can be using labels to mark a given PR as "intended for release" and architects can use labels to mark it approved, or just approve that PR. I don't want to prescribe a solution I just want to call out that this has quickly become difficult to keep up with at the PR level

[tjprescott]

I am not sure waiting for X is feasible since the doc mentions architects will be set as CodeOwners of the api.md file and it also said that the api.md file will change with every pr (that touches public api surface) enforced by the ci. So the architect will be pinged on every one of those to review.

Ah, good point. It would make more sense then to not put the architects as codeowners, but rather to simply assign them to review PRs. Since review PRs must be triggered (by the service team) this should eliminate that noise.

[tjprescott]

That's really not sustainable for us architects (assuming I understood the flow correctly). This used to be the case for JS (where we already generate api.md files under a review folder) but we were unable to keep up with the flood of api.md changes. A way to "batch" changes to a given review is really nice. Maybe that's something that can be using labels to mark a given PR as "intended for release" and architects can use labels to mark it approved, or just approve that PR. I don't want to prescribe a solution I just want to call out that this has quickly become difficult to keep up with at the PR level

Totally fair point, but this same limitation exists today in APIView.

You still could review changes in batches (features), but ultimately, you'll need to approve the actual release in its own PR. Most likely the service team would say "Here my PR for version 2. It consists of features A, B and C, and here are the approved PRs that contained those features."

It's also worth noting that Github has a feature that creates "groups" of changes to help break up large reviews.

[lmazuel]

For Python, I wanted it part of azpysdk apiview regenerate (not sure the exact name Jenny used). I agree we need to make this simple, likely integrate it into the azsdk CLI the EngSys team is already making available in all repos.

[tjprescott]

I have also tested that you can just ask @copilot to fix it on your PR and it was able to successfully do it. It takes a lot longer than just running the update on your machine, but it works.

[lmazuel]

This is a good point, I wonder what our architect would say about that. Some of those things could be done through a VSCode extension for instance.

[tjprescott]

GHC comments do seem to have a kind of severity badge on them. It doesn't seem to be a feature of manual comments right now, but it's possible that it may evolve in that direction.

[m-nash]

We can always do something like this.

• 🔴 High — bugs, security, correctness
• 🟡 Medium — improvements, inconsistencies
• 🔵 Low / ℹ️ — questions, suggestions

It would be really nice if GH exposed the ability to use the same severity marker copilot has access to. Do we know anyone over at GH?

[heaths]

In the short term, we could do something like what I do in some PRs e.g., preface a comment with "Tip: " or "Nit: ". Having something structured - even better, having access to what Copilot can do - would be better.

[lmazuel]

Tentative timeline?

[m-nash]

I am not sure that mitigation will work since a user can simply convert a draft pr to ready for review at which point ga approval would be granted even though beta approval was asked for?

I wonder if we should simply not require approvals if the release artifact will be beta except for the very first one which is what happens today. Approvals are only needed for ga.

[tjprescott]

Yeah, the intent it is to replicate the same logic. The concern here is that you want to release a GA, but there are two PRs, one that uses another GA version as the base and the other that uses a preview version as a base. The target is the same for both so approving either would ungate the release. However, I think we could examine the base and make sure that the GA approval is compared to a GA base and ignore if the base was a beta.

[heaths]

It's not uncommon for a team to be working on a stable and prerelease at the same time. I'm working with a partner right now doing that. I think we need to think more about feature/release branches and how we can utilize those e.g., as base branches rather than just the last tagged release.

[tjprescott]

That's a great point. There shouldn't be any reason base couldn't be a branch (though it probably introduces some complexity).

[JonathanGiles]

We presumably have one file per language - so do we end up with <service>/api.java.md or <service>/java/api.md?

[tjprescott]

The idea is that api.md will live in the package folder. So, for Python it's like sdk\storage\azure-storage-blob\api.md

[heaths]

Why wouldn't these live in language repos, thus not need the language name as the file name? That is where the package source is, so why shouldn't the API "source" be there as well?

[johanste]

And to clarify the assumptions; it will live in the target langauge repository, not in azure-rest-api-specs, correct?

[tjprescott]

That's correct. The api files would be in the language repos. There is a section that talks about potentially mirroring them to rest-api-specs so that you could look at all languages in one place, but that's not what is being proposed for phase 1.

[heaths]

There is a section that talks about potentially mirroring them to rest-api-specs so that you could look at all languages in one place

I hadn't gotten that far yet when I originally commented, but I do like that idea.

[JonathanGiles]

I'm not clear yet on how these PRs will be created. I imagine there are two flows (at least): for teams who are building generated libraries from typespec, and for teams who have built a custom library.

[tjprescott]

Essentially they will trigger some kind of script--most likely via the SDK agent. But of course anything you can trigger via an agent is usually exposed through a CLI command or pipeline. Right now in the Python POC PR, it is a script you could call (but again, it will probably be the agent calling it).

[JonathanGiles]

Having to have docs fully on or fully off is quite a big loss. I often expand the JavaDoc for an API to understand what the API does, but browsing with all apidocs showing will be horrendously bad for some libraries - and having to do essentially separate reviews on two files will be annoying too.

[tjprescott]

It might be that reviewing in the IDE could be a solution here--leveraging the ability of VSCode to collapse doc-comment blocks.

Just tried this... and it didn't work.

[JonathanGiles]

This will be a big change for Java and require a lot of work to move our diagnostics, which we invested heavily in, out of APIView and into separate CI-time validation.

[tjprescott]

100%. This will also affect C#, but won't affect other languages that don't use APIView diagnostics.

Since the current proposal leverages the token files to produce the text, it means the diagnostics would be available in some form. I think the simplest thing would be to extract them and have the CI log them as warnings. Other language CIs fail on any linter violations, but this wouldn't be possible since the diagnostics lack a suppression mechanism.

Longer term, the benefit of migrating the rules to a popular static analyzer of choice is that you can simply fail the CI and leverage the suppression mechanism of said analyzer.

[heaths]

What's the point of a markdown file? If you're not going to prescribe something more structured, why force a markdown file at all? What value does it add? .NET, for example, also publishes 1 or more .cs files that is basically just a large file of sorted declarations (with bodies that throw so it's valid C# and syntax highlights correctly). I could hypothetically do the same for Rust with a giant .rs file.

Is it just to follow a consistent naming pattern? Couldn't it just as easily be API.*? If it has to be markdown and there is no prescribed structural guidance, what stopping us just from:

```rust
// everything declaration goes here
```

[heaths]

Long-lived and/or disparate features should probably be done in feature branches anyway, but then that begs the question I asked above: how does that work with any gates to release from a feature (or release) branch?

[heaths]

Are we going to hook the current Copilot bot that APIView uses (or similar) to this process? It does provide value.

[tjprescott]

We are not. The current plan is for APIView Copilot to be retired alongside APIView.

[heaths]

Anything new to replace it? Barring some issues I had with our token stream (our processor's issue), it was actually useful. As we look at doing more with less, this would be a major boon. I've already spent a lot of time making sure our guidelines are up-to-date and actionable, so at least having Copilot take a first stab at review would likely save me a lot of time and, naturally - especially on larger PRs - we humans can miss things.

[tjprescott]

https://github.com/Azure/azure-sdk-tools/pull/15789/changes#diff-a88a3ba2b82577dec91ba21e12de184cfce3caa1af5b5ecee6f3de3b9dfbe3feR277

There's a section on AI-assisted reviews. Essentially, instead of a large black box bespoke solution, each language repo would have skills to facilitate reviews. While these cannot be used by the Github code review agent today, it's likely they will in the future.

[heaths]

Why wouldn't these live in language repos, thus not need the language name as the file name? That is where the package source is, so why shouldn't the API "source" be there as well?

[heaths]

In the short term, we could do something like what I do in some PRs e.g., preface a comment with "Tip: " or "Nit: ". Having something structured - even better, having access to what Copilot can do - would be better.

[heaths]

Nit, though I'm not sure there's a better way: this is going to look bad, IMO. There will be a lot of red Xes in our list of PRs that seems like, well, bad PR (public relations) unless people know what they are. Perhaps if the title is obvious that it's for review purposes only or something that would at least help the optics.

[tjprescott]

I mean, they can be merged. It would just merge the review branch into the base branch... which would do nothing because both branches will be deleted at some point.

[heaths]

It's not uncommon for a team to be working on a stable and prerelease at the same time. I'm working with a partner right now doing that. I think we need to think more about feature/release branches and how we can utilize those e.g., as base branches rather than just the last tagged release.

[heaths]

I'd righten up that wording to "regular approvals" since a PR comment sign-off is a thing, just as you noted above. That basically helps nothing. If you're signing off with suggestions, best to actually approve.

[tjprescott]

Well, except if you are doing suggestions, you'll often submit a review as "Comment" to ensure the team actually goes through them before you approve. That's how I suspect people would end up using that, but it's really up to the architect.

[heaths]

That would be "Request changes" then. Practically, they are the same.

But I'm - and seems many other people - hoping that is relaxed. Gone, unfortunately, are the days of "I don't see this as a blocker, but here's a few suggestions." Meaning, they can decide to ignore those and ship and we don't have to see another review.

Signing off as a Comment isn't signing off. Comment in this new world really is pretty useless.

[tjprescott]

Comment in this new world really is pretty useless.

Totally agree. 😁

But yeah, I think it'll be up to the architect. If you have suggestions and don't want to see the review again, leave the suggestions and approve. If you want to get some kind of reply (maybe you have questions) then you could just Comment.

[JonathanGiles]

I wonder how much value we would get from vibe coding a vscode plugin to make the reviews simpler? For example, it could unify docs/no-docs views, it might be able to display some degree of criticality on comments, it could automate some of the scripts / mcp / etc tooling processes.

[tjprescott]

What's the point of a markdown file? If you're not going to prescribe something more structured, why force a markdown file at all? What value does it add? .NET, for example, also publishes 1 or more .cs files that is basically just a large file of sorted declarations (with bodies that throw so it's valid C# and syntax highlights correctly). I could hypothetically do the same for Rust with a giant .rs file.

Is it just to follow a consistent naming pattern? Couldn't it just as easily be API.*? If it has to be markdown and there is no prescribed structural guidance, what stopping us just from:

```rust
// everything declaration goes here

The markdown today DOES look exactly as you suggest. It's certainly not set in stone, but a couple reasons:

1. the language tags you show mean that when viewing the API with markdown viewer, you'll get syntax highlighting (though this is obviously irrelevant when diffing)
2. Making the files with language extensions (api.py for example) is often incorrect. The Python APIView is not syntactically correct Python at all. It's basically very Python-like pseudocode. If you make the extension *.py then the GHC Review Agent will pollute the review with all kinds of erroneous Python syntax errors (as happens today in APIView!) and if you open in the IDE you area likely to get a bunch of distracting squiggles and warnings as it complains about your non-compliant code.