Re: [PR] feat(ci): add cargo-public-api checks for public API stability [iceberg-rust]
blackmwk merged PR #2525: URL: https://github.com/apache/iceberg-rust/pull/2525 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(ci): add cargo-public-api checks for public API stability [iceberg-rust]
blackmwk commented on code in PR #2525: URL: https://github.com/apache/iceberg-rust/pull/2525#discussion_r3324403912 ## .github/workflows/public-api.yml: ## @@ -0,0 +1,63 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: Public API Review 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(ci): add cargo-public-api checks for public API stability [iceberg-rust]
blackmwk commented on code in PR #2525: URL: https://github.com/apache/iceberg-rust/pull/2525#discussion_r3324398834 ## .github/workflows/public-api.yml: ## @@ -0,0 +1,63 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: Public API + +on: + push: +branches: + - main + pull_request: +paths: + - 'crates/**' Review Comment: Make sense to me since currently we don't publish `pyiceberg_core` as public api. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(ci): add cargo-public-api checks for public API stability [iceberg-rust]
xanderbailey commented on code in PR #2525: URL: https://github.com/apache/iceberg-rust/pull/2525#discussion_r3324298986 ## .github/workflows/public-api.yml: ## @@ -0,0 +1,63 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: Public API + +on: + push: +branches: + - main + pull_request: +paths: + - 'crates/**' Review 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` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(ci): add cargo-public-api checks for public API stability [iceberg-rust]
xanderbailey commented on code in PR #2525: URL: https://github.com/apache/iceberg-rust/pull/2525#discussion_r3324269848 ## .github/workflows/public-api.yml: ## @@ -0,0 +1,63 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: Public API + +on: + push: +branches: + - main + pull_request: +paths: + - 'crates/**' + - 'Cargo.toml' + - 'Cargo.lock' Review Comment: [9e36f09](https://github.com/apache/iceberg-rust/pull/2525/commits/9e36f09d1f130b98c51074069832db3877313b36) ## crates/integrations/cache-moka/public-api.txt: ## Review Comment: I like that idea also. [be7063a](https://github.com/apache/iceberg-rust/pull/2525/commits/be7063a71901f336b4a196dbdcca56751b03a100) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(ci): add cargo-public-api checks for public API stability [iceberg-rust]
xanderbailey commented on code in PR #2525: URL: https://github.com/apache/iceberg-rust/pull/2525#discussion_r3324257431 ## .github/workflows/public-api.yml: ## @@ -0,0 +1,63 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: Public API + +on: + push: +branches: + - main + pull_request: +paths: + - 'crates/**' + - 'Cargo.toml' + - 'Cargo.lock' Review Comment: Ah this was a copy paste from audit.yml, apologies, have removed -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(ci): add cargo-public-api checks for public API stability [iceberg-rust]
blackmwk commented on code in PR #2525: URL: https://github.com/apache/iceberg-rust/pull/2525#discussion_r3323666282 ## .github/workflows/public-api.yml: ## @@ -0,0 +1,63 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: Public API + +on: + push: +branches: + - main + pull_request: +paths: + - 'crates/**' Review Comment: `bindings/python` is missing. ## .github/workflows/public-api.yml: ## @@ -0,0 +1,63 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: Public API + +on: + push: +branches: + - main + pull_request: +paths: + - 'crates/**' + - 'Cargo.toml' + - 'Cargo.lock' Review Comment: Why we need this? ## public-api/iceberg-cache-moka.txt: ## Review Comment: I prefer to put a `public-api.txt` file in each crate's root dir, similar to what we did for DEPENDENCIES -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(ci): add cargo-public-api checks for public API stability [iceberg-rust]
Kurtiscwright commented on code in PR #2525: URL: https://github.com/apache/iceberg-rust/pull/2525#discussion_r3321095713 ## .github/workflows/public-api.yml: ## @@ -0,0 +1,63 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: Public API Review 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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(ci): add cargo-public-api checks for public API stability [iceberg-rust]
xanderbailey commented on PR #2525: URL: https://github.com/apache/iceberg-rust/pull/2525#issuecomment-4568508561 @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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
[PR] feat(ci): add cargo-public-api checks for public API stability [iceberg-rust]
xanderbailey opened a new pull request, #2525: URL: https://github.com/apache/iceberg-rust/pull/2525 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 ## Which issue does this PR close? - Closes #. ## What changes are included in this PR? ## Are these changes tested? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
