🐛 Update the testing so that addon-analyzer is built correctly#1109
Conversation
…image that is under test Signed-off-by: Shawn Hurley <shawn@hurley.page>
📝 WalkthroughWalkthroughThe demo-testing workflow is modified to replace the Dockerfile base image reference during the build step, changing it from a remote public registry image to a locally-tagged image generated from the current build's image tag. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/demo-testing.yml (1)
331-332: The sed pattern will successfully match the actual FROM line; consider adding explicit verification for robustnessThe
tackle2-addon-analyzerDockerfile contains exactlyFROM quay.io/konveyor/analyzer-lsp:lateston line 6, so the sed substitution will succeed. The pattern is correct.However, two minor improvements:
- The
\.in the regex pattern is unnecessary;.inquay.ioandkonveyorare unescaped and match any character rather than a literal dot (though unlikely to cause issues in practice).- The
\:escape is redundant;:doesn't require escaping in sed.While the substitution will work with the external repository as currently configured, adding an explicit check that the replacement occurred (e.g.,
grep -q "FROM localhost/konveyor-analyzer-lsp:...") would prevent silent failures if the Dockerfile is updated in the future to use a different tag, digest, or build-stage alias.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/demo-testing.yml around lines 331 - 332, The sed replacement using the command that targets the Dockerfile (the line with FROM quay.io/konveyor/analyzer-lsp:latest) should be followed by an explicit verification step to ensure the substitution actually occurred; after running the sed command that edits Dockerfile, run a check (e.g., grep -q for the expected "FROM localhost/konveyor-analyzer-lsp:${{ needs.detect-changes.outputs.image_tag }}" string) and fail the job if the grep does not find the replacement, so the subsequent IMG=... make image-podman invocation does not proceed on a silent no-op; keep the existing sed target but add the verification and non-zero exit on failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/demo-testing.yml:
- Around line 331-332: The sed replacement using the command that targets the
Dockerfile (the line with FROM quay.io/konveyor/analyzer-lsp:latest) should be
followed by an explicit verification step to ensure the substitution actually
occurred; after running the sed command that edits Dockerfile, run a check
(e.g., grep -q for the expected "FROM localhost/konveyor-analyzer-lsp:${{
needs.detect-changes.outputs.image_tag }}" string) and fail the job if the grep
does not find the replacement, so the subsequent IMG=... make image-podman
invocation does not proceed on a silent no-op; keep the existing sed target but
add the verification and non-zero exit on failure.
| - name: Build addon and push images | ||
| working-directory: tackle2-addon-analyzer | ||
| run: | | ||
| sed -i "s,FROM quay.io/konveyor/analyzer-lsp\:latest,FROM localhost/konveyor-analyzer-lsp:${{ needs.detect-changes.outputs.image_tag }}," Dockerfile |
There was a problem hiding this comment.
I think using a build arg is probably the better way to do this but minor nit
There was a problem hiding this comment.
LGTM if you want to just get it in
There was a problem hiding this comment.
I agree with you, I do have a action item for myself to have some org wide thing that will make this more standard to make the image build process's easier
Currently, the addon image is built from the latest on all workflows; it should be built from the image under test.
Summary by CodeRabbit