feat(ci): add cargo-public-api checks for public API stability#2525
Conversation
Add infrastructure to track and enforce the public API surface of all published crates. This catches unintentional breaking changes in PRs by diffing the current API against committed lock files. - Add `public-api/` directory with lock files for 10 published crates - Add GitHub Actions workflow that runs on crate/dependency changes - Add `make generate-public-api` and `make check-public-api` targets - Exclude generated lock files from license header checks
xanderbailey
force-pushed
the
add-public-api-checks
branch
from
5ee8c0c to
f8f24b1
Compare
|
@liurenjie1024 As discussed in the community sync, thought I'd put up a PR to get the conversation going. Let me know what you think |
Kurtiscwright
left a comment
Kurtiscwright
left a comment
There was a problem hiding this comment.
Thank you for finding this concept and getting this PR out so fast, very cool stuff.
| # specific language governing permissions and limitations | ||
| # under the License. | ||
|
|
||
| name: Public API |
There was a problem hiding this comment.
Does this make sense right now when making breaking changes is to some degree expected, though no desirable? I like the Makefile so its simple to get feedback about breaking changes, but to have this in CI seems premature especially with the CI rules around fast failing.
There was a problem hiding this comment.
I think the workflow is used to ensure that the public-api doc is in sync, so that when anything changes, the reviewers can be notified. It's not used to ensure api not changing.
blackmwk
left a comment
blackmwk
left a comment
There was a problem hiding this comment.
Thanks @xanderbailey for this pr, I love this idea! Left some comments to address.
| - main | ||
| pull_request: | ||
| paths: | ||
| - 'crates/**' |
There was a problem hiding this comment.
bindings/python is missing.
There was a problem hiding this comment.
do we need python bindings to trigger this? I'm not sure that can cause a public api break in the sense that we're only checking for rust api breaks with cargo-public-api
There was a problem hiding this comment.
Make sense to me since currently we don't publish pyiceberg_core as public api.
| - 'Cargo.toml' | ||
| - 'Cargo.lock' |
There was a problem hiding this comment.
Ah this was a copy paste from audit.yml, apologies, have removed
There was a problem hiding this comment.
I prefer to put a public-api.txt file in each crate's root dir, similar to what we did for DEPENDENCIES
blackmwk
left a comment
blackmwk
left a comment
There was a problem hiding this comment.
Thanks @xanderbailey for this pr!
dannycjones
left a comment
dannycjones
left a comment
There was a problem hiding this comment.
Nice! Thanks for adding this, it will make API changes so much clearer.
4 participants
Add infrastructure to track and enforce the public API surface of all published crates. This catches unintentional breaking changes in PRs by diffing the current API against committed lock files.
public-api/directory with lock files for 10 published cratesmake generate-public-apiandmake check-public-apitargetsWhich issue does this PR close?
What changes are included in this PR?
Are these changes tested?