zeroshade commented on code in PR #1: URL: https://github.com/apache/iceberg-terraform/pull/1#discussion_r2615463569
########## .github/workflows/go-ci.yml: ########## @@ -0,0 +1,59 @@ +# 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: Go + +on: + push: + branches: + - 'main' + tags: + - 'v**' + pull_request: + +concurrency: + group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{ github.workflow }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +permissions: + contents: read + +jobs: + lint-and-test: + name: ${{ matrix.os }} go${{ matrix.go }} + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + go: [ '1.23.6', '1.24.9' ] Review Comment: ```suggestion go: [ '1.25'] ``` the go.mod has 1.25.1, indicating that this module is only valid for 1.25.1+ In that case we shouldn't bother testing 1.23 and 1.24 here. If you want to maintain compatibility with the older versions, then shouldn't we drop the version in the go.mod? ########## .github/workflows/go-ci.yml: ########## @@ -0,0 +1,59 @@ +# 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: Go + +on: + push: + branches: + - 'main' + tags: + - 'v**' + pull_request: + +concurrency: + group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{ github.workflow }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +permissions: + contents: read + +jobs: + lint-and-test: + name: ${{ matrix.os }} go${{ matrix.go }} + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + go: [ '1.23.6', '1.24.9' ] + os: [ 'ubuntu-latest', 'windows-latest', 'macos-latest' ] + steps: + - uses: actions/checkout@v4 + - name: Install Go + uses: actions/setup-go@f111f3307d8850f501ac008e886eec1fd1932a34 # v5.3.0 + with: + go-version: ${{ matrix.go }} + cache: true + cache-dependency-path: go.sum + - name: Install tools + run: make install-tools + - name: Go mod tidy + run: make tidy + - name: Lint + run: make lint + - name: Run tests + run: make test Review Comment: just use the `go test -v` command directly here ########## .github/workflows/go-ci.yml: ########## @@ -0,0 +1,59 @@ +# 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: Go + +on: + push: + branches: + - 'main' + tags: + - 'v**' + pull_request: + +concurrency: + group: ${{ github.repository }}-${{ github.head_ref || github.sha }}-${{ github.workflow }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +permissions: + contents: read + +jobs: + lint-and-test: + name: ${{ matrix.os }} go${{ matrix.go }} + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + go: [ '1.23.6', '1.24.9' ] + os: [ 'ubuntu-latest', 'windows-latest', 'macos-latest' ] + steps: + - uses: actions/checkout@v4 + - name: Install Go + uses: actions/setup-go@f111f3307d8850f501ac008e886eec1fd1932a34 # v5.3.0 + with: + go-version: ${{ matrix.go }} + cache: true + cache-dependency-path: go.sum + - name: Install tools + run: make install-tools + - name: Go mod tidy + run: make tidy + - name: Lint + run: make lint Review Comment: can we use pre-commit for this instead? It would be cleaner and we wouldn't need the Makefile (I find the `.pre-commit-config.yaml` a much better way to do this. We should probably also add a `.golangci.yml` config file ########## internal/provider/provider_test.go: ########## @@ -0,0 +1,31 @@ +// 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. + +package provider + +import ( + "os" + "testing" +) + +func TestMain(m *testing.M) { + os.Exit(m.Run()) +} + +func TestProvider(t *testing.T) { + if New() == nil { + t.Fatal("Expected New() to return a non-nil provider") + } Review Comment: should we use `github.com/stretchr/testify/assert`? we could do `assert.NotNil(t, New())` ########## internal/provider/provider_test.go: ########## @@ -0,0 +1,31 @@ +// 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. + +package provider + +import ( + "os" + "testing" +) + +func TestMain(m *testing.M) { + os.Exit(m.Run()) +} Review Comment: this is unnecessary if we're not going to customize the actual test running ########## main.go: ########## @@ -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. + +package main + +import ( + "context" + "log" + + "github.com/apache/iceberg-terraform/internal/provider" + "github.com/hashicorp/terraform-plugin-framework/providerserver" +) + +func main() { + err := providerserver.Serve(context.Background(), provider.New, providerserver.ServeOpts{ + // TODO: This needs to change on release with the published name. + Address: "iceberg.apache.org/terraform", Review Comment: we could make this a constant that would be easy to script updating on releases? -- 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]
