add github action that run e2e test on windows #336
Conversation
WalkthroughAdds two GitHub Actions workflows: one builds a Windows AMD64 e2e OCI image and uploads it as an artifact; the other downloads that artifact, provisions a Windows instance/container, runs macadam e2e tests in a matrix, uploads reports, updates PR status, and performs cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant GH_Actions as GitHub Actions
participant Artifact as Artifact Store
participant Podman as Podman/Container Host
participant Mapt as mapt (VM image builder)
participant Test_Container as Windows test container
participant GitHub_API as GitHub Status API
GH_Actions->>Podman: Build Windows AMD64 e2e image (make containerized_e2e)
Podman->>Artifact: Save image tar and upload artifact
GH_Actions->>Artifact: Download e2e image artifact (matrix job)
GH_Actions->>GitHub_API: Create pending PR status (commit_sha, context)
GH_Actions->>Mapt: Create Windows desktop instance (version, featurepack, secrets)
Mapt->>Podman: Start Windows instance (podman run)
GH_Actions->>Test_Container: Load image and run macadam e2e tests
Test_Container->>Artifact: Produce junit/xml reports and logs
GH_Actions->>GitHub_API: Update PR status (success/failure)
GH_Actions->>Mapt: Destroy instance and cleanup (final)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/e2e-test-windows.yaml:
- Around line 52-65: The workflow references undefined inputs (inputs.qe-type
and inputs.preset) causing empty values; fix by either adding a
workflow_dispatch with inputs for qe-type and preset (so inputs.qe-type and
inputs.preset are defined), or convert this to a workflow_call with explicit
inputs, or replace all occurrences of `${{inputs.qe-type}}`/`${{inputs.preset}}`
with concrete defaults (e.g., "e2e" and a chosen preset) or env vars.
Specifically update the trigger block to include workflow_dispatch or
workflow_call inputs if you want dynamic values, or edit the status payload
construction (`data` string using "${{inputs.qe-type}}-${{inputs.preset}}"), the
project/name string usages, and artifact naming sites to use the newly defined
inputs or the chosen hardcoded/default values so names/descriptions are never
empty.
- Around line 1-9: Fix two typos in the workflow comments: update the first
comment that mentions "triger" to "trigger" in the header block near the
workflow name windows-e2e, and correct "sucess" to "success" in the permissions
comment (the comment explaining permission settings). Leave all other text
unchanged.
- Around line 37-50: Assign the GitHub Action expressions into quoted shell
variables and write them safely to GITHUB_ENV to avoid shell injection: change
the assignments to use quotes around the expansions (e.g., commit_sha="${{
github.event.workflow_run.head_commit.id }}" and
status_context="ci/gh/e2e/windows-${{ matrix.windows-version }}-${{
matrix.windows-featurepack }}"), keep the null/empty checks using the quoted
variable (${commit_sha}), and append to GITHUB_ENV with a safe formatter like
printf '%s=%s\n' "commit_sha" "$commit_sha" and printf '%s=%s\n'
"status_context" "$status_context" instead of unquoted echo.
🧹 Nitpick comments (6)
.github/workflows/e2e-image.yml (2)
1-12: Consider removing thetestbranch trigger before merging.Line 6 includes the
testbranch in push triggers. This appears to be for development/testing purposes. If so, consider removing it before merging tomainto avoid running the workflow unnecessarily on a scratch branch.
17-18: Unnecessarystrategyblock.
strategyandfail-fast: falsehave no effect without amatrix. Since this is a single-configuration job, the block can be removed.Proposed fix
build-qe: runs-on: ubuntu-latest - strategy: - fail-fast: false steps:.github/workflows/e2e-test-windows.yaml (4)
53-59:set -xuois incomplete andset -xmay expose sensitive data in logs.Line 54:
set -xuoshould beset -xuo pipefailto catch failures in piped commands. Also,set -xtraces every command to the log — while this step doesn't directly use secrets, it's worth being cautious. The same applies to line 140.Proposed fix
- set -xuo + set -xuo pipefail
94-114: Hardcoded S3 URL for the Fedora image.Line 99 uses a hardcoded S3 URL for the Fedora cloud image. Consider extracting this to a workflow-level
envvariable or a repository variable for easier updates when the image version changes.
137-154: Unquoted expression on line 143.
${{steps.test-report.outcome}}is injected unquoted into the shell. While its value set is constrained (success/failure/etc.), quoting it is safer and consistent with best practices.Proposed fix
- if [[ ${{steps.test-report.outcome}} != "success" ]]; then + if [[ "${{ steps.test-report.outcome }}" != "success" ]]; then
156-174: Cleanup step looks good —if: always()ensures resource teardown.The destroy step correctly runs unconditionally and removes Pulumi locks before tearing down. However, there's no error handling if
podman logs -f windows-destroyfails or the destroy itself fails — consider adding a timeout or retry to avoid dangling Azure resources.
| - name: Correlate | ||
| run: | | ||
| # Get origin commit sha for testing | ||
| commit_sha=${{ github.event.workflow_run.head_commit.id }} | ||
| if [[ -z "${commit_sha}" ]] || [[ "${commit_sha}" == null ]]; then | ||
| # if .head_commit.id is empty, then .head_sha is used as commit instead | ||
| commit_sha=${{ github.event.workflow_run.head_sha }} | ||
| fi | ||
| echo "commit_sha=${commit_sha}" >> "$GITHUB_ENV" | ||
|
|
||
| # Set status_context | ||
| status_context="ci/gh/e2e" | ||
| status_context="${status_context}/windows-${{matrix.windows-version}}-${{matrix.windows-featurepack}}" | ||
| echo "status_context=${status_context}" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
Unquoted expressions are a script injection risk.
Lines 40 and 43 inject ${{ ... }} directly into the shell without quoting. While commit SHAs are typically safe, best practice is to pass them via environment variables to prevent injection if the value is ever attacker-controlled.
Proposed fix — use intermediate env vars
- name: Correlate
+ env:
+ HEAD_COMMIT_ID: ${{ github.event.workflow_run.head_commit.id }}
+ HEAD_SHA: ${{ github.event.workflow_run.head_sha }}
run: |
# Get origin commit sha for testing
- commit_sha=${{ github.event.workflow_run.head_commit.id }}
- if [[ -z "${commit_sha}" ]] || [[ "${commit_sha}" == null ]]; then
+ commit_sha="${HEAD_COMMIT_ID}"
+ if [[ -z "${commit_sha}" ]] || [[ "${commit_sha}" == "null" ]]; then
# if .head_commit.id is empty, then .head_sha is used as commit instead
- commit_sha=${{ github.event.workflow_run.head_sha }}
+ commit_sha="${HEAD_SHA}"
fi
echo "commit_sha=${commit_sha}" >> "$GITHUB_ENV"🤖 Prompt for AI Agents
In @.github/workflows/e2e-test-windows.yaml around lines 37 - 50, Assign the
GitHub Action expressions into quoted shell variables and write them safely to
GITHUB_ENV to avoid shell injection: change the assignments to use quotes around
the expansions (e.g., commit_sha="${{ github.event.workflow_run.head_commit.id
}}" and status_context="ci/gh/e2e/windows-${{ matrix.windows-version }}-${{
matrix.windows-featurepack }}"), keep the null/empty checks using the quoted
variable (${commit_sha}), and append to GITHUB_ENV with a safe formatter like
printf '%s=%s\n' "commit_sha" "$commit_sha" and printf '%s=%s\n'
"status_context" "$status_context" instead of unquoted echo.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @.github/workflows/e2e-test-windows.yaml:
- Around line 156-174: The destroy step uses a detached container (podman run -d
--name windows-destroy) so failures don't propagate and several env vars
(ARM_TENANT_ID, ARM_SUBSCRIPTION_ID, ARM_CLIENT_ID) are unquoted; change the
podman invocation in the "Destroy instance" step to run foreground (remove -d)
so the step returns the container exit code (e.g., podman run --name
windows-destroy --rm ...), and ensure all secret env assignments use the same
quoted form as the others ('${{ secrets.ARM_TENANT_ID }}', '${{
secrets.ARM_SUBSCRIPTION_ID }}', '${{ secrets.ARM_CLIENT_ID }}') so values with
special chars are safe; keep podman logs -f windows-destroy only if you still
need live logs, or remove it since the foreground run will stream logs and the
step will fail on non-zero exit.
- Around line 102-114: The step uses a detached podman run (--rm -d) named
windows-e2e-test followed by podman logs -f, which hides the container's exit
code; change the flow so you either remove --rm and keep -d then after podman
logs -f call podman wait/inspect on container windows-e2e-test to capture its
exit code (and then remove the container), or run podman in foreground (drop -d
and logs -f) so the command's exit status reflects the test result; apply the
same fix for the other steps that use podman run --rm -d and podman logs -f
(Create Windows instance and Destroy instance).
- Line 54: The shell invocation uses an incomplete flag sequence "set -xuo"
which passes -o without an option; update the occurrences of the "set -xuo"
command to "set -xuo pipefail" so -o gets the intended "pipefail" argument
(ensure you replace both occurrences of the literal "set -xuo" in the workflow
file).
- Around line 5-13: The workflow currently triggers on workflow_run types:
[completed] and will run regardless of the triggering workflow's result; update
the workflow trigger or the job to only proceed when the triggering workflow
succeeded by adding a condition that checks github.event.workflow_run.conclusion
== 'success' (for example add if: github.event.workflow_run.conclusion ==
'success' to the windows-qe job or gate the whole workflow_run trigger
accordingly) so the windows-qe job (and its Azure VM startup) only runs when the
Build e2e image workflow completed successfully.
- Around line 73-78: The -e environment flags use inconsistent quoting for
secrets (ARM_TENANT_ID, ARM_SUBSCRIPTION_ID, ARM_CLIENT_ID vs ARM_CLIENT_SECRET,
AZURE_STORAGE_ACCOUNT, AZURE_STORAGE_KEY); update the workflow so all -e secret
references are consistently quoted (e.g., single-quote or double-quote each ${ {
secrets.* } } expansion) to prevent shell metacharacter issues, and apply the
same change to the destroy step where the same variables are set.
🧹 Nitpick comments (2)
.github/workflows/e2e-test-windows.yaml (2)
69-69:chmod -R 777on the entire workspace is overly permissive.This grants world read/write/execute to every file in
$GITHUB_WORKSPACE. Consider scoping the permission change to only the directories themaptcontainer needs to write to, or use a more restrictive mode like755/775.
143-143: Unquoted${{ steps.test-report.outcome }}in shell conditional.While the value is constrained to a known set, it's best practice to pass it via an environment variable or at least quote it in the shell to be consistent with injection-safe patterns.
Proposed fix
+ env: + TEST_OUTCOME: ${{ steps.test-report.outcome }} run: | set -xuo pipefail # Status msg data="{\"state\":\"success\"" - if [[ ${{steps.test-report.outcome}} != "success" ]]; then + if [[ "${TEST_OUTCOME}" != "success" ]]; then data="{\"state\":\"failure\"" fi
| -e ARM_TENANT_ID=${{secrets.ARM_TENANT_ID}} \ | ||
| -e ARM_SUBSCRIPTION_ID=${{secrets.ARM_SUBSCRIPTION_ID}} \ | ||
| -e ARM_CLIENT_ID=${{secrets.ARM_CLIENT_ID}} \ | ||
| -e ARM_CLIENT_SECRET='${{secrets.ARM_CLIENT_SECRET}}' \ | ||
| -e AZURE_STORAGE_ACCOUNT='${{ secrets.AZURE_STORAGE_ACCOUNT }}' \ | ||
| -e AZURE_STORAGE_KEY='${{ secrets.AZURE_STORAGE_KEY }}' \ |
There was a problem hiding this comment.
Inconsistent quoting of secrets in -e flags.
ARM_CLIENT_SECRET, AZURE_STORAGE_ACCOUNT, and AZURE_STORAGE_KEY are single-quoted, but ARM_TENANT_ID, ARM_SUBSCRIPTION_ID, and ARM_CLIENT_ID are not. If any unquoted secret contains shell metacharacters, it will break or be misinterpreted. Apply consistent quoting to all secret references.
Proposed fix
- -e ARM_TENANT_ID=${{secrets.ARM_TENANT_ID}} \
- -e ARM_SUBSCRIPTION_ID=${{secrets.ARM_SUBSCRIPTION_ID}} \
- -e ARM_CLIENT_ID=${{secrets.ARM_CLIENT_ID}} \
+ -e ARM_TENANT_ID='${{secrets.ARM_TENANT_ID}}' \
+ -e ARM_SUBSCRIPTION_ID='${{secrets.ARM_SUBSCRIPTION_ID}}' \
+ -e ARM_CLIENT_ID='${{secrets.ARM_CLIENT_ID}}' \Same inconsistency exists in the destroy step (lines 164–166).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -e ARM_TENANT_ID=${{secrets.ARM_TENANT_ID}} \ | |
| -e ARM_SUBSCRIPTION_ID=${{secrets.ARM_SUBSCRIPTION_ID}} \ | |
| -e ARM_CLIENT_ID=${{secrets.ARM_CLIENT_ID}} \ | |
| -e ARM_CLIENT_SECRET='${{secrets.ARM_CLIENT_SECRET}}' \ | |
| -e AZURE_STORAGE_ACCOUNT='${{ secrets.AZURE_STORAGE_ACCOUNT }}' \ | |
| -e AZURE_STORAGE_KEY='${{ secrets.AZURE_STORAGE_KEY }}' \ | |
| -e ARM_TENANT_ID='${{secrets.ARM_TENANT_ID}}' \ | |
| -e ARM_SUBSCRIPTION_ID='${{secrets.ARM_SUBSCRIPTION_ID}}' \ | |
| -e ARM_CLIENT_ID='${{secrets.ARM_CLIENT_ID}}' \ | |
| -e ARM_CLIENT_SECRET='${{secrets.ARM_CLIENT_SECRET}}' \ | |
| -e AZURE_STORAGE_ACCOUNT='${{ secrets.AZURE_STORAGE_ACCOUNT }}' \ | |
| -e AZURE_STORAGE_KEY='${{ secrets.AZURE_STORAGE_KEY }}' \ |
🤖 Prompt for AI Agents
In @.github/workflows/e2e-test-windows.yaml around lines 73 - 78, The -e
environment flags use inconsistent quoting for secrets (ARM_TENANT_ID,
ARM_SUBSCRIPTION_ID, ARM_CLIENT_ID vs ARM_CLIENT_SECRET, AZURE_STORAGE_ACCOUNT,
AZURE_STORAGE_KEY); update the workflow so all -e secret references are
consistently quoted (e.g., single-quote or double-quote each ${ { secrets.* } }
expansion) to prevent shell metacharacter issues, and apply the same change to
the destroy step where the same variables are set.
| podman run --rm -d --name windows-e2e-test \ | ||
| -e OS=windows \ | ||
| -e TARGET_HOST=$(cat host) \ | ||
| -e TARGET_HOST_USERNAME=$(cat username) \ | ||
| -e TARGET_HOST_KEY_PATH=/data/id_rsa \ | ||
| -e TARGET_FOLDER=macadam-test \ | ||
| -e TARGET_RESULTS=results \ | ||
| -e OUTPUT_FOLDER=/data \ | ||
| -e DEBUG=true \ | ||
| -v $PWD:/data:z \ | ||
| macadam-e2e:windows-amd64 \ | ||
| ${cmd} | ||
| podman logs -f windows-e2e-test |
There was a problem hiding this comment.
Detached podman run -d + podman logs -f does not propagate the container's exit code.
podman logs -f will exit 0 regardless of whether the container succeeded or failed. If the e2e test container crashes or exits non-zero, this step will still appear to succeed. You should capture the container's exit code after logs finish.
This same pattern affects the "Create Windows instance" step (lines 71/91) and the "Destroy instance" step (lines 162/174).
Proposed fix for this step
podman run --rm -d --name windows-e2e-test \
...
${cmd}
podman logs -f windows-e2e-test
+ podman wait windows-e2e-test || true
+ exit $(podman inspect windows-e2e-test --format='{{.State.ExitCode}}')Note: Since --rm is set, the container is removed after exit, so podman wait/inspect won't work. Either remove --rm and clean up manually, or switch to foreground execution (drop -d and logs -f).
🤖 Prompt for AI Agents
In @.github/workflows/e2e-test-windows.yaml around lines 102 - 114, The step
uses a detached podman run (--rm -d) named windows-e2e-test followed by podman
logs -f, which hides the container's exit code; change the flow so you either
remove --rm and keep -d then after podman logs -f call podman wait/inspect on
container windows-e2e-test to capture its exit code (and then remove the
container), or run podman in foreground (drop -d and logs -f) so the command's
exit status reflects the test result; apply the same fix for the other steps
that use podman run --rm -d and podman logs -f (Create Windows instance and
Destroy instance).
| - name: Destroy instance | ||
| if: always() | ||
| run: | | ||
| # Make sure lock is removed | ||
| rm -rf .pulumi/locks/* | ||
| # Destroy | ||
| podman run -d --name windows-destroy --rm \ | ||
| -v ${PWD}:/workspace:z \ | ||
| -e ARM_TENANT_ID=${{secrets.ARM_TENANT_ID}} \ | ||
| -e ARM_SUBSCRIPTION_ID=${{secrets.ARM_SUBSCRIPTION_ID}} \ | ||
| -e ARM_CLIENT_ID=${{secrets.ARM_CLIENT_ID}} \ | ||
| -e ARM_CLIENT_SECRET='${{secrets.ARM_CLIENT_SECRET}}' \ | ||
| -e AZURE_STORAGE_ACCOUNT='${{ secrets.AZURE_STORAGE_ACCOUNT }}' \ | ||
| -e AZURE_STORAGE_KEY='${{ secrets.AZURE_STORAGE_KEY }}' \ | ||
| quay.io/redhat-developer/mapt:v0.9.9 azure \ | ||
| windows destroy \ | ||
| --project-name 'windows-desktop-${{ matrix.windows-version }}-${{ matrix.windows-featurepack }}-macadam' \ | ||
| --backed-url azblob://crc-qenvs-state/${{ github.repository }}-${{ github.run_id }} | ||
| podman logs -f windows-destroy |
There was a problem hiding this comment.
Destroy step has the same exit-code and secret-quoting issues noted above.
The detached run + logs pattern won't propagate a destroy failure, and ARM_TENANT_ID/ARM_SUBSCRIPTION_ID/ARM_CLIENT_ID remain unquoted (lines 164–166). If destroy fails silently, the Azure VM will leak and continue incurring costs.
🤖 Prompt for AI Agents
In @.github/workflows/e2e-test-windows.yaml around lines 156 - 174, The destroy
step uses a detached container (podman run -d --name windows-destroy) so
failures don't propagate and several env vars (ARM_TENANT_ID,
ARM_SUBSCRIPTION_ID, ARM_CLIENT_ID) are unquoted; change the podman invocation
in the "Destroy instance" step to run foreground (remove -d) so the step returns
the container exit code (e.g., podman run --name windows-destroy --rm ...), and
ensure all secret env assignments use the same quoted form as the others ('${{
secrets.ARM_TENANT_ID }}', '${{ secrets.ARM_SUBSCRIPTION_ID }}', '${{
secrets.ARM_CLIENT_ID }}') so values with special chars are safe; keep podman
logs -f windows-destroy only if you still need live logs, or remove it since the
foreground run will stream logs and the step will fail on non-zero exit.
1 participant
Summary by CodeRabbit
New Features
Tests
Chores