laskoviymishka commented on code in PR #5: URL: https://github.com/apache/iceberg-verification/pull/5#discussion_r4024385214
########## .github/workflows/ci.yml: ########## @@ -0,0 +1,35 @@ +# 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: CI + +on: Review Comment: Since this is the base future runners build on, I'd add a `merge_group` trigger here alongside push/pull_request. This repo's `.asf.yaml` sets `required_linear_history` + `squash`, which pairs with GitHub's merge queue — without `merge_group`, required checks won't fire when a PR enters the queue, so it can stall or slip through unverified. iceberg-python and iceberg-rust both declare it. Cheap to add while the trigger block is still small: ```yaml on: push: branches: - main pull_request: merge_group: ``` wdyt? ########## .github/workflows/ci.yml: ########## @@ -0,0 +1,35 @@ +# 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: CI + +on: + push: + branches: + - main + pull_request: Review Comment: Small asymmetry here: push is scoped to `main` but `pull_request` has no `branches` filter, so it fires on PRs against any branch. Harmless today, but once release/integration branches show up, every PR against them runs this (and whatever jobs land later). I'd either add `branches: [main]` or drop a comment that the broader scope is intentional — either's fine, just so it's a deliberate choice rather than an oversight. wdyt? ########## .github/workflows/ci.yml: ########## @@ -0,0 +1,35 @@ +# 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: CI + +on: + push: + branches: + - main + pull_request: + +permissions: + contents: read Review Comment: While we're setting the base, I'd add a `concurrency` block — every sibling Iceberg repo (iceberg-python, iceberg-rust) defines one, and once real jobs land, rapid fixup pushes will queue duplicate runs on shared ASF capacity. ```suggestion contents: read concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: ${{ github.event_name == 'pull_request' }} ``` Cheapest to bake in now so the future runners inherit it. ########## .github/workflows/ci.yml: ########## @@ -0,0 +1,35 @@ +# 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: CI + +on: + push: + branches: + - main + pull_request: + +permissions: + contents: read + +jobs: + bootstrap: + name: bootstrap + runs-on: ubuntu-latest Review Comment: Minor, but I'd pin `ubuntu-24.04` over the floating `ubuntu-latest` — the alias silently rolls to a new OS when GitHub flips it, which can break tool assumptions in the real jobs later with no change to the workflow. iceberg-rust pins its runners for exactly this. ```suggestion runs-on: ubuntu-24.04 ``` ########## .github/workflows/ci.yml: ########## @@ -0,0 +1,35 @@ +# 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: CI + +on: + push: + branches: + - main + pull_request: + +permissions: + contents: read + +jobs: + bootstrap: + name: bootstrap + runs-on: ubuntu-latest + steps: + - name: No-op + run: echo "CI bootstrap check." Review Comment: One thought on the no-op: a bare `echo` really only proves the YAML parses and a runner starts. Since the point is a foundation the real jobs inherit, it might be worth having it `actions/checkout@v4` so we're actually exercising repo access and token scopes that every downstream job needs (and it'd be the first thing to shake out the allowlist gap I flagged in the summary). Totally fine to keep it minimal and land the scaffold first if you'd rather — wdyt? -- 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]
