Repository navigation
Modernize Docker packaging: drop stale images, fix untested test stages - #1995
Merged
Merged
Conversation
…st stages Remove unused Dockerfiles targeting EOL runtimes (.NET 5/6, .NET Core 3.1, Amazon Linux Lambda, App Engine aspnetcore 3.1) that are not referenced by any CI workflow and were only generating Renovate noise. Also drop the superseded ubuntu24-dotnet10-opencv4.13.0 (+slim) images now that main targets OpenCV 5. Point docker-deploy.yml's manual publish workflow at the current ubuntu24-dotnet10-opencv5.0.0 image instead of the retired 4.13.0 one. Fix docker-test-ubuntu.yml so the Dockerfile's test-native/test-dotnet stages actually run: they were unreachable from the default build target, so BuildKit silently skipped them and the "Docker Test" CI never verified anything beyond the final image assembling correctly. Add explicit --target builds for both stages. This also surfaced that test-dotnet referenced the renamed OpenCvSharp.Extensions -> OpenCvSharp.GdipExtensions project and built it against the wrong TargetFramework; fixed to match the projects' actual net8.0/net10.0 targets.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe deploy workflow now points to the ubuntu24-dotnet10-opencv5.0.0 image. The OpenCV 5.0.0 Dockerfile updates its CMake flags and test-dotnet builds. The docker-test workflow adds cache-only native and .NET test build steps. ChangesDocker image target and CI updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant build_full
participant build_slim
participant DockerBuildxCache
GitHubActions->>build_full: run test-native cache-only build
build_full->>DockerBuildxCache: cache-to scoped full image layers
GitHubActions->>build_full: run test-dotnet cache-only build
build_full->>DockerBuildxCache: cache-to scoped full image layers
GitHubActions->>build_slim: run test-native cache-only build
build_slim->>DockerBuildxCache: cache-to scoped slim image layers
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/docker-test-ubuntu.yml (1)
27-66: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache-to scope reused across steps overwrites earlier caches.
The new
test-nativeandtest-dotnetsteps and the existing final build step all writecache-towith the identical scopefull-${{ github.ref_name }}. Per Docker's own docs, writing to the same gha cache scope more than once in a run means only the last write's cache survives, so the earliercache-towrites here are effectively discarded — reducing the caching benefit these new steps were meant to provide for subsequent runs.Consider giving each step its own scope suffix (e.g.,
full-test-native-${{ github.ref_name }},full-test-dotnet-${{ github.ref_name }}) and listing all relevant scopes in each step'scache-from.♻️ Example scope differentiation
- name: Test native .so (full) uses: docker/build-push-action@v7 with: context: . file: ${{ env.DOCKER_BUILD_CONTEXT_FULL }}/Dockerfile target: test-native build-args: | OPENCV_VERSION=${{ env.OPENCV_VERSION }} outputs: type=cacheonly cache-from: | type=gha,scope=full-${{ github.ref_name }} type=gha,scope=full-main - cache-to: type=gha,mode=min,scope=full-${{ github.ref_name }} + cache-to: type=gha,mode=min,scope=full-test-native-${{ github.ref_name }} - name: Test .NET class libraries (full) uses: docker/build-push-action@v7 with: context: . file: ${{ env.DOCKER_BUILD_CONTEXT_FULL }}/Dockerfile target: test-dotnet build-args: | OPENCV_VERSION=${{ env.OPENCV_VERSION }} outputs: type=cacheonly - cache-from: | - type=gha,scope=full-${{ github.ref_name }} - type=gha,scope=full-main - cache-to: type=gha,mode=min,scope=full-${{ github.ref_name }} + cache-from: | + type=gha,scope=full-test-native-${{ github.ref_name }} + type=gha,scope=full-main + cache-to: type=gha,mode=min,scope=full-test-dotnet-${{ github.ref_name }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/docker-test-ubuntu.yml around lines 27 - 66, The Docker cache writes in the workflow are using the same gha scope across multiple steps, so later `cache-to` writes overwrite earlier ones. Update the `docker/build-push-action@v7` steps for `test-native`, `test-dotnet`, and the final build to use unique cache scope suffixes, and adjust their `cache-from` entries to include the relevant scopes so each step can reuse the others’ caches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/docker-test-ubuntu.yml:
- Around line 78-91: The slim test step is sharing the same GitHub Actions cache
scope as the other slim build step, so its cache gets overwritten instead of
being stored independently. Update the cache settings in the test-native
build-push-action block to use a distinct scope for the slim test job, and keep
the existing slim build scope separate so each step writes and restores its own
cache cleanly.
---
Nitpick comments:
In @.github/workflows/docker-test-ubuntu.yml:
- Around line 27-66: The Docker cache writes in the workflow are using the same
gha scope across multiple steps, so later `cache-to` writes overwrite earlier
ones. Update the `docker/build-push-action@v7` steps for `test-native`,
`test-dotnet`, and the final build to use unique cache scope suffixes, and
adjust their `cache-from` entries to include the relevant scopes so each step
can reuse the others’ caches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a46384d8-1131-44fa-95b7-74fd831a669f
📒 Files selected for processing (11)
.github/workflows/docker-deploy.yml.github/workflows/docker-test-ubuntu.ymlpackaging/docker/al2-dotnet5-opencv4.6.0/Dockerfilepackaging/docker/al2-opencv4.5.1/Dockerfilepackaging/docker/appengine-aspnetcore3.1-opencv4.5.1/Dockerfilepackaging/docker/ubuntu22-dotnet6-opencv4.8.0/Dockerfilepackaging/docker/ubuntu22-dotnet6sdk-opencv4.7.0/Dockerfilepackaging/docker/ubuntu24-dotnet10-opencv4.13.0-slim/Dockerfilepackaging/docker/ubuntu24-dotnet10-opencv4.13.0/Dockerfilepackaging/docker/ubuntu24-dotnet10-opencv5.0.0/Dockerfilepackaging/docker/ubuntu24-dotnet8-opencv4.12.0/Dockerfile
💤 Files with no reviewable changes (8)
- packaging/docker/ubuntu22-dotnet6sdk-opencv4.7.0/Dockerfile
- packaging/docker/ubuntu24-dotnet8-opencv4.12.0/Dockerfile
- packaging/docker/al2-dotnet5-opencv4.6.0/Dockerfile
- packaging/docker/appengine-aspnetcore3.1-opencv4.5.1/Dockerfile
- packaging/docker/ubuntu24-dotnet10-opencv4.13.0-slim/Dockerfile
- packaging/docker/ubuntu22-dotnet6-opencv4.8.0/Dockerfile
- packaging/docker/ubuntu24-dotnet10-opencv4.13.0/Dockerfile
- packaging/docker/al2-opencv4.5.1/Dockerfile
…d cache test-native/test-dotnet steps shared the same GHA cache scope as the subsequent final image build, so their cache-to write got clobbered by the later build's cache-to save, evicting the test-only layers on every run. Give each test step its own scope while still reading from the shared scope for common base layers.
… the extern link The now-actually-running test-native stage caught a pre-existing bug: OpenCV 5.0.0's vendored MLAS declares and calls MlasHGemmSupported() on its FP16/GQA path but never defines it, leaving libOpenCvSharpExtern.so with an undefined reference. windows.yml/linux-arm64.yml/macos.yml already work around this by dropping CMAKE_ASM_COMPILER so MLAS reports itself unavailable and DNN falls back to its built-in SGEMM; apply the same fix here.
4 of 5 tasks
3 of 4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ubuntu24-dotnet10-opencv4.13.0(+-slim) images now thatmaintargets OpenCV 5.docker-deploy.yml's manual publish workflow atubuntu24-dotnet10-opencv5.0.0instead of the retired 4.13.0 image.docker-test-ubuntu.yml: the Dockerfile'stest-native/test-dotnetstages were unreachable from the default build target, so BuildKit silently skipped them — the "Docker Test" CI never actually verified the native.soor randotnet testinside the image. Added explicit--targetbuild steps for both stages so they run and can fail the job.test-dotnetitself, which referenced the renamedOpenCvSharp.Extensionsproject (nowOpenCvSharp.GdipExtensions) and built against the wrongTargetFramework; now matches the projects' actualnet8.0/net10.0targets.Test plan
Docker Testworkflow passes on this PR (both full and slim jobs), including the newtest-native/test-dotnettarget stepsdocker-deploy.ymlstill validates (workflow_dispatch input check) — not runnable from a PR, but review the diff for correctness🤖 Generated with Claude Code
Summary by CodeRabbit