test: reenable ui e2e tests + improvements#1206
test: reenable ui e2e tests + improvements#1206rsoaresd merged 8 commits intocodeready-toolchain:masterfrom
Conversation
WalkthroughAdds CI/PR-aware image naming and two public Make variables in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as CI Env / Local
participant Make as make/sandbox-ui.mk
participant Quay as Quay Registry
CI->>Make: invoke build/push (envs vary)
alt CI with CLONEREFS_OPTIONS + GITHUB_ACTIONS
Make->>Make: IMAGE = ${IMAGE_NAME_TO_PUSH_IN_QUAY}:from.${REPOSITORY_NAME}.PR${PULL_NUMBER}.${COMMIT_ID_SUFFIX}
else CI with CLONEREFS_OPTIONS (no GITHUB_ACTIONS)
alt REPO_NAME unset
Make->>Make: IMAGE = quay.io/codeready-toolchain/sandbox-rhdh-plugin:latest
else REPO_NAME set
Make->>Make: IMAGE = ${IMAGE_NAME_TO_PUSH_IN_QUAY}:from.${REPO_NAME}.PR${PULL_NUMBER}.${COMMIT_ID_SUFFIX}
end
else Local / Non-CI
Make->>Make: IMAGE = ${IMAGE_NAME_TO_PUSH_IN_QUAY}:latest
end
alt When PUSH_SANDBOX_IMAGE=true
Make->>Quay: push IMAGE
end
sequenceDiagram
autonumber
participant Dev as Developer/CI
participant Make as make (test-e2e)
participant Deps as existing e2e prereqs
participant UI as test-ui-e2e
Dev->>Make: run target test-e2e
Make->>Deps: run existing e2e prerequisites
Make->>UI: run test-ui-e2e (CI may set PUSH_SANDBOX_IMAGE=true)
UI-->>Make: return UI results
Deps-->>Make: return other results
Make-->>Dev: aggregate and report results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (2)
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
| COMMIT_ID_SUFFIX=$$(echo "$${PULL_PULL_SHA}" | cut -c1-7); \ | ||
| echo "from.$(REPO_NAME).PR$(PULL_NUMBER).$${COMMIT_ID_SUFFIX}"; \ | ||
| if [ -z "$(REPO_NAME)" ]; then \ | ||
| echo "quay.io/codeready-toolchain/sandbox-rhdh-plugin:v49"; \ |
There was a problem hiding this comment.
same used in sandbox-sre
There was a problem hiding this comment.
How would we make sure the periodic jobs are always using the latest version of the plugin? Is there a latest tag? If not, should we create one?
There was a problem hiding this comment.
This is the "same problem" we have right now in sandbox-sre, we need to be sure that it is updated to the latest version and not forget to update it (not ideal, of course). Regarding creating a latest tag, we can discuss it with the team. TBH, I do not know the reason why we do not have it
There was a problem hiding this comment.
The main reason is because we want to be able to first deploy the image on staging and only after testing and validating there we want to update production.
More details about how the CD works now https://github.com/codeready-toolchain/sandbox-sre/blob/master/components/rhdh/README.md
There was a problem hiding this comment.
That said , we could also build a tag with latest evertime we update the image, and use this only for e2e testing maybe.
There was a problem hiding this 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: 1
🧹 Nitpick comments (2)
make/test.mk (1)
45-45: UI E2E now part of test-e2e — ensure deterministic order (avoid -j hazards).As added,
test-ui-e2eis just another prerequisite and may run in parallel ifmake -jis used. If ordering matters (it likely does), either:
- Mark the target non-parallel, or
- Invoke
$(MAKE) test-ui-e2eas the last recipe step instead of a prerequisite.To prevent parallel execution for this target, add:
.NOTPARALLEL: test-e2eAlternatively, move
test-ui-e2eout of the prerequisite list and call it explicitly at the end of thetest-e2erecipe.make/sandbox-ui.mk (1)
13-14: Good addition; minor reuse improvement.Consider deriving the registry path from the existing plugin name variable to avoid duplication.
Apply this small tweak:
-IMAGE_NAME_TO_PUSH_IN_QUAY ?= quay.io/$(QUAY_NAMESPACE)/sandbox-rhdh-plugin +IMAGE_NAME_TO_PUSH_IN_QUAY ?= quay.io/$(QUAY_NAMESPACE)/$(SANDBOX_PLUGIN_IMAGE_NAME)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
make/sandbox-ui.mk(1 hunks)make/test.mk(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Unit Tests
- GitHub Check: GolangCI Lint
- GitHub Check: Build & push operator bundles for e2e tests
- GitHub Check: Build & push Developer Sandbox UI image for UI e2e tests
| # if $(REPO_NAME) is not set, it means that the E2E tests were triggered by periodic CI job | ||
| IMAGE_TO_PUSH_IN_QUAY := $(shell \ | ||
| if [ -n "$(CI)$(CLONEREFS_OPTIONS)" ]; then \ | ||
| if [ -n "$(GITHUB_ACTIONS)" ]; then \ | ||
| REPOSITORY_NAME=$$(basename "$(GITHUB_REPOSITORY)"); \ | ||
| COMMIT_ID_SUFFIX=$$(echo "$(PULL_PULL_SHA)" | cut -c1-7); \ | ||
| echo "from.$${REPOSITORY_NAME}.PR$(PULL_NUMBER).$${COMMIT_ID_SUFFIX}"; \ | ||
| echo "$(IMAGE_NAME_TO_PUSH_IN_QUAY):from.$${REPOSITORY_NAME}.PR$(PULL_NUMBER).$${COMMIT_ID_SUFFIX}"; \ | ||
| else \ | ||
| AUTHOR=$$(jq -r '.refs[0].pulls[0].author' <<< $${CLONEREFS_OPTIONS} | tr -d '[:space:]'); \ | ||
| PULL_PULL_SHA=$${PULL_PULL_SHA:-$$(jq -r '.refs[0].pulls[0].sha' <<< $${CLONEREFS_OPTIONS} | tr -d '[:space:]')}; \ | ||
| COMMIT_ID_SUFFIX=$$(echo "$${PULL_PULL_SHA}" | cut -c1-7); \ | ||
| echo "from.$(REPO_NAME).PR$(PULL_NUMBER).$${COMMIT_ID_SUFFIX}"; \ | ||
| if [ -z "$(REPO_NAME)" ]; then \ | ||
| echo "quay.io/codeready-toolchain/sandbox-rhdh-plugin:v49"; \ | ||
| else \ | ||
| AUTHOR=$$(jq -r '.refs[0].pulls[0].author' <<< $${CLONEREFS_OPTIONS} | tr -d '[:space:]'); \ | ||
| PULL_PULL_SHA=$${PULL_PULL_SHA:-$$(jq -r '.refs[0].pulls[0].sha' <<< $${CLONEREFS_OPTIONS} | tr -d '[:space:]')}; \ | ||
| COMMIT_ID_SUFFIX=$$(echo "$${PULL_PULL_SHA}" | cut -c1-7); \ | ||
| echo "$(IMAGE_NAME_TO_PUSH_IN_QUAY):from.$(REPO_NAME).PR$(PULL_NUMBER).$${COMMIT_ID_SUFFIX}"; \ | ||
| fi; \ | ||
| fi; \ | ||
| else \ | ||
| echo "latest"; \ | ||
| echo "$(IMAGE_NAME_TO_PUSH_IN_QUAY):latest"; \ | ||
| fi) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Fix bashism and brittle CI fallbacks in IMAGE_TO_PUSH_IN_QUAY computation.
- Bash-only here-strings (
<<<) will fail under/bin/sh(common in CI). Pipe JSON to jq instead. - Avoid hardcoded
v49; use the existingTAGor make it overrideable. - Handle GitHub Actions periodic runs (no PULL_NUMBER/PULL_PULL_SHA): fallback to a stable tag.
- Allow explicit override by callers (
?=instead of:=).
Apply this diff:
-# if $(REPO_NAME) is not set, it means that the E2E tests were triggered by periodic CI job
-IMAGE_TO_PUSH_IN_QUAY := $(shell \
+# if $(REPO_NAME) is not set, it means that the E2E tests were triggered by periodic CI job
+IMAGE_TO_PUSH_IN_QUAY ?= $(shell \
if [ -n "$(CI)$(CLONEREFS_OPTIONS)" ]; then \
if [ -n "$(GITHUB_ACTIONS)" ]; then \
REPOSITORY_NAME=$$(basename "$(GITHUB_REPOSITORY)"); \
- COMMIT_ID_SUFFIX=$$(echo "$(PULL_PULL_SHA)" | cut -c1-7); \
- echo "$(IMAGE_NAME_TO_PUSH_IN_QUAY):from.$${REPOSITORY_NAME}.PR$(PULL_NUMBER).$${COMMIT_ID_SUFFIX}"; \
+ COMMIT=$${PULL_PULL_SHA:-$${GITHUB_SHA}}; \
+ COMMIT_ID_SUFFIX=$$(echo "$${COMMIT}" | cut -c1-7); \
+ PULL_NO=$${PULL_NUMBER:-$$(echo "$${GITHUB_REF}" | sed -n 's#refs/pull/\([0-9]\+\)/.*#\1#p')}; \
+ if [ -n "$${PULL_NO}" ] && [ -n "$${COMMIT_ID_SUFFIX}" ]; then \
+ echo "$(IMAGE_NAME_TO_PUSH_IN_QUAY):from.$${REPOSITORY_NAME}.PR$${PULL_NO}.$${COMMIT_ID_SUFFIX}"; \
+ else \
+ echo "quay.io/codeready-toolchain/sandbox-rhdh-plugin:$(TAG)"; \
+ fi; \
else \
if [ -z "$(REPO_NAME)" ]; then \
- echo "quay.io/codeready-toolchain/sandbox-rhdh-plugin:v49"; \
+ echo "quay.io/codeready-toolchain/sandbox-rhdh-plugin:$(TAG)"; \
else \
- AUTHOR=$$(jq -r '.refs[0].pulls[0].author' <<< $${CLONEREFS_OPTIONS} | tr -d '[:space:]'); \
- PULL_PULL_SHA=$${PULL_PULL_SHA:-$$(jq -r '.refs[0].pulls[0].sha' <<< $${CLONEREFS_OPTIONS} | tr -d '[:space:]')}; \
+ AUTHOR=$$(echo "$${CLONEREFS_OPTIONS}" | jq -r '.refs[0].pulls[0].author' | tr -d '[:space:]'); \
+ PULL_PULL_SHA=$${PULL_PULL_SHA:-$$(echo "$${CLONEREFS_OPTIONS}" | jq -r '.refs[0].pulls[0].sha' | tr -d '[:space:]')}; \
COMMIT_ID_SUFFIX=$$(echo "$${PULL_PULL_SHA}" | cut -c1-7); \
echo "$(IMAGE_NAME_TO_PUSH_IN_QUAY):from.$(REPO_NAME).PR$(PULL_NUMBER).$${COMMIT_ID_SUFFIX}"; \
fi; \
fi; \
else \
echo "$(IMAGE_NAME_TO_PUSH_IN_QUAY):latest"; \
fi)Notes:
- If you need a pinned tag for periodic jobs, consider parameterizing via
TAGor a dedicated variable instead of hardcoding. - If GH-based pairing requires
AUTHOR, set it fromGITHUB_ACTORin the GH path separately.
Please confirm:
- CI uses
/bin/sh(not bash). If yes, the above change is required to avoid syntax errors. - That
quay.io/codeready-toolchain/sandbox-rhdh-plugin:$(TAG)exists for periodic runs (or provide the tag viaTAG=<value>).
🏁 Script executed:
#!/bin/bash
# Verify bash-only here-strings in sandbox-ui.mk
rg -n '<<<' make/sandbox-ui.mk
# Check if TAG is defined anywhere in the repo
rg -nP '^\s*TAG\s*[:?]?=' .Length of output: 268
Replace bashisms with POSIX-compliant syntax and parameterize TAG
- Use
?=forIMAGE_TO_PUSH_IN_QUAYto allow overrides. - Replace
<<<here-strings withecho "$${CLONEREFS_OPTIONS}" | jq -r …. - Swap hardcoded
:v49to:$(TAG)(defaults tolatestat line 3). - In GitHub Actions branch, if
PULL_NUMBERor commit SHA is unset, fallback to$(TAG)instead of failing.
| COMMIT_ID_SUFFIX=$$(echo "$${PULL_PULL_SHA}" | cut -c1-7); \ | ||
| echo "from.$(REPO_NAME).PR$(PULL_NUMBER).$${COMMIT_ID_SUFFIX}"; \ | ||
| if [ -z "$(REPO_NAME)" ]; then \ | ||
| echo "quay.io/codeready-toolchain/sandbox-rhdh-plugin:v49"; \ |
There was a problem hiding this comment.
How would we make sure the periodic jobs are always using the latest version of the plugin? Is there a latest tag? If not, should we create one?
|
/retest likely is flaky, i will keep an eye |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/e2e/sandbox-ui/README.md (1)
19-19: Docs match behavior of test-ui-e2e-local.Dropping PUSH_SANDBOX_IMAGE from the local command aligns with the Make target. Consider mentioning QUAY_NAMESPACE default and that podman login to quay.io is required for pushes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
make/sandbox-ui.mk(2 hunks)test/e2e/sandbox-ui/README.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & push Developer Sandbox UI image for UI e2e tests
- GitHub Check: Build & push operator bundles for e2e tests
🔇 Additional comments (3)
make/sandbox-ui.mk (3)
13-15: Good extraction of registry/namespace defaults.Defining QUAY_NAMESPACE and IMAGE_NAME_TO_PUSH_IN_QUAY upfront improves clarity and reuse.
205-205: Nice: local target auto-pushes plugin image.Inline PUSH_SANDBOX_IMAGE=true in test-ui-e2e-local matches the docs change and reduces user friction.
If desired, add a short README note that a podman login to quay.io is required before running this.
16-35: Fix bashisms, remove hardcoded tag, and harden GH/periodic fallbacks in IMAGE_TO_PUSH_IN_QUAY.
- Here-strings (<<<) are bash-only; Make defaults to /bin/sh in CI.
- Hardcoded v49 makes periodic jobs brittle; parameterize via TAG.
- In GH Actions, PULL_PULL_SHA/PULL_NUMBER may be unset; add fallbacks to GITHUB_SHA/GITHUB_REF.
- Allow explicit overrides of IMAGE_TO_PUSH_IN_QUAY.
Apply:
-IMAGE_TO_PUSH_IN_QUAY := $(shell \ +IMAGE_TO_PUSH_IN_QUAY ?= $(shell \ if [ -n "$(CI)$(CLONEREFS_OPTIONS)" ]; then \ if [ -n "$(GITHUB_ACTIONS)" ]; then \ REPOSITORY_NAME=$$(basename "$(GITHUB_REPOSITORY)"); \ - COMMIT_ID_SUFFIX=$$(echo "$(PULL_PULL_SHA)" | cut -c1-7); \ - echo "$(IMAGE_NAME_TO_PUSH_IN_QUAY):from.$${REPOSITORY_NAME}.PR$(PULL_NUMBER).$${COMMIT_ID_SUFFIX}"; \ + COMMIT=$${PULL_PULL_SHA:-$${GITHUB_SHA}}; \ + COMMIT_ID_SUFFIX=$$(echo "$${COMMIT}" | cut -c1-7); \ + PULL_NO=$${PULL_NUMBER:-$$(echo "$${GITHUB_REF}" | sed -n 's#refs/pull/\([0-9]\+\)/.*#\1#p')}; \ + if [ -n "$${PULL_NO}" ] && [ -n "$${COMMIT_ID_SUFFIX}" ]; then \ + echo "$(IMAGE_NAME_TO_PUSH_IN_QUAY):from.$${REPOSITORY_NAME}.PR$${PULL_NO}.$${COMMIT_ID_SUFFIX}"; \ + else \ + echo "quay.io/codeready-toolchain/sandbox-rhdh-plugin:$(TAG)"; \ + fi; \ else \ : "if REPO_NAME is not set, it means that the E2E tests were triggered by periodic CI job"; \ if [ -z "$(REPO_NAME)" ]; then \ - echo "quay.io/codeready-toolchain/sandbox-rhdh-plugin:v49"; \ + echo "quay.io/codeready-toolchain/sandbox-rhdh-plugin:$(TAG)"; \ else \ - AUTHOR=$$(jq -r '.refs[0].pulls[0].author' <<< $${CLONEREFS_OPTIONS} | tr -d '[:space:]'); \ - PULL_PULL_SHA=$${PULL_PULL_SHA:-$$(jq -r '.refs[0].pulls[0].sha' <<< $${CLONEREFS_OPTIONS} | tr -d '[:space:]')}; \ + AUTHOR=$$(echo "$${CLONEREFS_OPTIONS}" | jq -r '.refs[0].pulls[0].author' | tr -d '[:space:]'); \ + PULL_PULL_SHA=$${PULL_PULL_SHA:-$$(echo "$${CLONEREFS_OPTIONS}" | jq -r '.refs[0].pulls[0].sha' | tr -d '[:space:]')}; \ COMMIT_ID_SUFFIX=$$(echo "$${PULL_PULL_SHA}" | cut -c1-7); \ echo "$(IMAGE_NAME_TO_PUSH_IN_QUAY):from.$(REPO_NAME).PR$(PULL_NUMBER).$${COMMIT_ID_SUFFIX}"; \ fi; \ fi; \ else \ echo "$(IMAGE_NAME_TO_PUSH_IN_QUAY):latest"; \ fi)Notes:
- If periodic runs must not use latest, set TAG in the job (e.g., TAG=v49) instead of hardcoding here.
- If pairing logic relies on AUTHOR in GH, consider
AUTHOR ?= $(GITHUB_ACTOR)at the top.Run to confirm remaining bashisms and hardcoded tags:
#!/bin/bash rg -n '<<<' make/sandbox-ui.mk || true rg -n 'sandbox-rhdh-plugin:v[0-9]+' make/sandbox-ui.mk || true rg -n 'IMAGE_TO_PUSH_IN_QUAY\s*:=' make/sandbox-ui.mk || true
| SSO_PASSWORD_READ := $(shell if [ -n "$(CI)" ]; then cat /usr/local/sandbox-secrets/SSO_PASSWORD 2>/dev/null || echo ""; else echo "${SSO_PASSWORD}"; fi) | ||
|
|
||
| TAG := $(shell \ | ||
| QUAY_NAMESPACE ?= codeready-toolchain-test |
There was a problem hiding this comment.
this is already set in test.mk - we don't need duplication
| QUAY_NAMESPACE ?= codeready-toolchain-test |
There was a problem hiding this comment.
I added it because there was a run that was considering it empty: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/codeready-toolchain_toolchain-e2e/1206/pull-ci-codeready-toolchain-toolchain-e2e-master-e2e/1971263226020630528/artifacts/e2e/gather-extra/artifacts/pods/sandbox-ui_rhdh-5c87cf9795-sf2wx_install-dynamic-plugins.log
There was a problem hiding this comment.
That's weird. Most likely it's because the test.mk is alphabetically after this one, so it was evaluated later.
| PULL_PULL_SHA=$${PULL_PULL_SHA:-$$(jq -r '.refs[0].pulls[0].sha' <<< $${CLONEREFS_OPTIONS} | tr -d '[:space:]')}; \ | ||
| COMMIT_ID_SUFFIX=$$(echo "$${PULL_PULL_SHA}" | cut -c1-7); \ | ||
| echo "from.$(REPO_NAME).PR$(PULL_NUMBER).$${COMMIT_ID_SUFFIX}"; \ | ||
| : "if REPO_NAME is not set, it means that the E2E tests were triggered by periodic CI job"; \ |
There was a problem hiding this comment.
| : "if REPO_NAME is not set, it means that the E2E tests were triggered by periodic CI job"; \ | |
| echo "if REPO_NAME is not set, it means that the E2E tests were triggered by periodic CI job"; \ |
There was a problem hiding this comment.
i used : in order to comment inside the shell
There was a problem hiding this comment.
ah, ok - I misunderstood that. I have never seen such a syntax, my bad.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MatousJobanek, rajivnathan, rsoaresd The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
|
/retest flaky test: |
|
/retest infra issue |
1 similar comment
|
/retest infra issue |
4 participants
Description
IMAGE_TO_PUSH_IN_QUAY, if we are running in periodic job we need to get from ourcodeready-toolchainquay orgSummary by CodeRabbit