GODRIVER-3493 Compile check all support Go versions #1992
Conversation
prestonvasquez
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a compilation check test to ensure the code compiles correctly across various Go versions using Docker images.
- Added a new test file (compile_check_test.go) that spins up containers with different Go versions and runs a compile check script.
- Updated Taskfile.yml to invoke the new compile check test script and adjusted the Evergreen configuration accordingly.
Reviewed Changes
Copilot reviewed 4 out of 8 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/test/compilecheck/compile_check_test.go | New test file that uses testcontainers to run compile checks with various Go images. |
| Taskfile.yml | Updated compile check commands to point to the new test script. |
| .evergreen/config.yml | Changed task reference to the updated compile check command. |
Files not reviewed (4)
- etc/compile_check.sh: Language not supported
- etc/run-compile-check-test.sh: Language not supported
- go.work: Language not supported
- internal/test/compilecheck/go.mod: Language not supported
mongodb-drivers-pr-bot
bot
added
dependencies
API Change ReportNo changes found! |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new compile check test to verify that the driver compiles against multiple Go versions. The changes include:
- Adding a new Go test file to run compile checks in Docker containers using different Go versions.
- Updating Taskfile.yml with new tasks to run the compile check tests.
- Adjusting the Evergreen configuration to execute the compile check for the minimum supported version.
Reviewed Changes
Copilot reviewed 4 out of 8 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/test/compilecheck/compile_check_test.go | New Go test to run compile checks across various Go versions. |
| Taskfile.yml | Updated build tasks switching to a new compile check script. |
| .evergreen/config.yml | Modified task arguments and expanded the target platforms. |
Files not reviewed (4)
- etc/compile_check.sh: Language not supported
- etc/run-compile-check-test.sh: Language not supported
- go.work: Language not supported
- internal/test/compilecheck/go.mod: Language not supported
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new compilation check test that validates driver compatibility against multiple Golang versions using test containers. Key changes include:
- Introducing a new compile check test in internal/test/compilecheck/compile_check_test.go that iterates over available Golang Docker images.
- Updating Taskfile.yml to replace the direct compile check call with a new shell script (run-compile-check-test.sh) and adding a separate task for the minimum supported version.
- Adjusting the Evergreen configuration to run the new build-compile-check-msv task on an updated host group.
Reviewed Changes
Copilot reviewed 4 out of 8 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/test/compilecheck/compile_check_test.go | Adds a new test using test containers to verify compilation across Go versions. |
| Taskfile.yml | Refactors compile check commands and adds a task specifically for the minimum supported version. |
| .evergreen/config.yml | Updates task configuration to use the new minimum supported version task. |
Files not reviewed (4)
- etc/compile_check.sh: Language not supported
- etc/run-compile-check-test.sh: Language not supported
- go.work: Language not supported
- internal/test/compilecheck/go.mod: Language not supported
Comments suppressed due to low confidence (2)
Taskfile.yml:35
- [nitpick] The task name 'build-compile-check-msv' and its argument 'TestCompileCheck/golang:1.18' might be unclear for future maintainers; consider adding an inline comment or using a more descriptive name to clearly indicate its purpose as running the compile check for the minimum supported version.
build-compile-check-msv: bash etc/run-compile-check-test.sh TestCompileCheck/golang:1.18
.evergreen/config.yml:1430
- [nitpick] Verify that the usage of 'build-compile-check-msv' in the Evergreen config matches the intended deployment environment and is consistently referenced across the infrastructure.
args: [*task-runner, build-compile-check-msv]
335e5bf to
a35b7c7
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds a compile check test to confirm the Go driver’s compatibility with all supported Go versions using Docker containers and updates related build tasks and CI configuration.
- Added a new test file (compile_check_test.go) that runs compile checks across different Golang Docker images.
- Introduced a new Taskfile task (build-compile-check-all) and updated Evergreen configuration to execute it.
Reviewed Changes
Copilot reviewed 4 out of 7 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/test/compilecheck/compile_check_test.go | New test to run compile checks against available Golang Docker images. |
| Taskfile.yml | Added a task to run all compile check tests. |
| .evergreen/config.yml | Updated CI configuration to run the new compile check task on an additional platform. |
Files not reviewed (3)
- etc/compile_check.sh: Language not supported
- etc/run-compile-check-test.sh: Language not supported
- internal/test/compilecheck/go.mod: Language not supported
prestonvasquez
changed the title
prestonvasquez
requested review from
a team and
qingyang-hu
and removed request for
a team
| return nil, fmt.Errorf("failed to decode response Body from Docker Hub: %w", err) | ||
| } | ||
|
|
||
| resp.Body.Close() |
There was a problem hiding this comment.
It's better to move the Body.Close() immediately after the error check of http.Get() and wrap it with a defer.
There was a problem hiding this comment.
Deferring here (in a loop) will cause body closures to stack. If we closed the body after the error check we wouldn't be able to decode into the data struct.
There was a problem hiding this comment.
Right... However, there is a risk that the current implementation may fail during decoding, leaving the body unclosed. Is it possible to wrap the HTTP Get in a separate function?
There was a problem hiding this comment.
I can update the code to close the response body if there is an error.
There was a problem hiding this comment.
It is somewhat hacky, but I think it's acceptable in a test...
GODRIVER-3493
Summary
Run compile check for all min versions greater than or equal to the minimum supported version.
Background & Motivation
Starting in Go 1.21, the compiler will refuse to use modules that declare a newer Go version. Generally, we should ensure the driver is compatible with all current Go versions.