SANDBOX-1371: prepare enablement of ui e2e tests#1180
SANDBOX-1371: prepare enablement of ui e2e tests#1180rsoaresd merged 32 commits intocodeready-toolchain:masterfrom
Conversation
| TESTS_RUN_FILTER_REGEXP ?= "" | ||
|
|
||
| .PHONY: test-e2e | ||
| test-e2e: test-ui-e2e |
There was a problem hiding this comment.
temporary, just to test
| # Execute composite action using values from PR event | ||
| # Is executed only for PR events - in that case the comment field is empty | ||
| - name: Publish current Developer Sandbox UI image | ||
| uses: rsoaresd/toolchain-cicd/publish-sandbox-ui-for-ui-e2e-tests@add_actions_to_ui_e2e_tests |
There was a problem hiding this comment.
temporary, just to test
| # Execute composite action using values from PR event | ||
| # Is executed only for comment events - in that case the pull_request field is empty | ||
| - name: Publish current Developer Sandbox UI image | ||
| uses: rsoaresd/toolchain-cicd/publish-sandbox-ui-for-ui-e2e-tests@add_actions_to_ui_e2e_tests |
There was a problem hiding this comment.
temporary, just to test
| # Execute composite action using values from PR event | ||
| # Is executed only for PR events - in that case the comment field is empty | ||
| - name: Publish current operator bundles for host & member based on PR event | ||
| uses: rsoaresd/toolchain-cicd/publish-operators-for-ui-e2e-tests@add_actions_to_ui_e2e_tests |
There was a problem hiding this comment.
temporary, just to test
| # Execute composite action using values from PR event | ||
| # Is executed only for comment events - in that case the pull_request field is empty | ||
| - name: Publish current operator bundles for host & member based on comment event | ||
| uses: rsoaresd/toolchain-cicd/publish-operators-for-ui-e2e-tests@add_actions_to_ui_e2e_tests |
There was a problem hiding this comment.
temporary, just to test
rsoaresd
changed the title
|
WalkthroughRemoved Node.js 22 and Yarn installation from Dockerfiles; added libX11-xcb to Firefox deps; removed registrationService auth from UI e2e ToolchainConfig; rewired sandbox UI Makefile for CI/PR TAG logic, SSO credential reads, rhdh-plugins pairing ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CI as CI / Developer
participant Make as make/sandbox-ui.mk
participant GH as GitHub Actions
participant Repo as rhdh-plugins
Note over Make: Build / tag decision
CI->>Make: invoke build/push
alt CI environment or CLONEREFS present
Make->>Make: compute TAG from PR context (GH or CLONEREFS)
Make-->>CI: set IMAGE_TO_PUSH_IN_QUAY with computed TAG
else Local
Make->>Make: set TAG = "latest"
end
Note over Make,Repo: rhdh-plugins pairing (pair-if-needed)
Make->>GH: detect author's fork/branch
alt author branch exists
Make->>Repo: clone, add remote, fetch branch, merge into tmp path
else
Make->>Repo: clone master into tmp path
end
Make-->>CI: continue build/push using TAG and optional PUSH_SANDBOX_IMAGE
sequenceDiagram
autonumber
actor Dev as Developer
participant Mk as make/test.mk
participant UI as test-ui-e2e
Dev->>Mk: make test-e2e
Mk->>UI: run test-ui-e2e (prerequisite)
UI-->>Dev: UI e2e result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
scripts/ci/manage-operator.sh (2)
81-93: Numeric compare uses string operator (may mis-detect multiple branches).Use -gt for integers.
- if [[ $(echo ${GET_BRANCH_NAME} | wc -l) > 1 ]]; then \ + if [[ $(echo "${GET_BRANCH_NAME}" | wc -l) -gt 1 ]]; then \
197-205: Uninstall wait loop can spin even when no pods exist.
oc get pods ...prints “No resources found…”, so[[ -n ... ]]stays true. Check for names instead.- while [[ -n `oc get pods -l control-plane=controller-manager -n ${NAMESPACE} 2>/dev/null || true` ]]; do + while [[ -n "$(oc get pods -l control-plane=controller-manager -n ${NAMESPACE} -o name 2>/dev/null)" ]]; domake/sandbox-ui.mk (1)
197-216: Avoid unconditional host-side plugin build in containerized run.This still runs on the host even when
PUSH_SANDBOX_IMAGE=falsefor the container.-.PHONY: test-sandbox-ui-in-container -test-sandbox-ui-in-container: build-sandbox-ui-e2e-tests - @echo "pushing Developer Sandbox UI image..." - $(MAKE) push-sandbox-plugin +.PHONY: test-sandbox-ui-in-container +test-sandbox-ui-in-container: build-sandbox-ui-e2e-tests +ifneq ($(PUSH_SANDBOX_IMAGE),false) + @echo "pushing Developer Sandbox UI image..." + $(MAKE) push-sandbox-plugin +else + @echo "skipping host-side plugin build (PUSH_SANDBOX_IMAGE=false)" +endif
♻️ Duplicate comments (1)
make/test.mk (1)
43-48: test-e2e now short-circuits to UI tests only — confirm pipeline expectations.This changes semantics and may surprise jobs invoking test-e2e for operator E2E.
Consider gating via a flag so CI can flip behavior:
.PHONY: test-e2e -test-e2e: test-ui-e2e +ENABLE_UI_E2E ?= true +test-e2e: $(if $(filter true,$(ENABLE_UI_E2E)),test-ui-e2e,) +# usage: make ENABLE_UI_E2E=false test-e2eTo find call sites relying on old behavior:
#!/bin/bash rg -nP -C2 '\b(make|/usr/bin/make)\b.*\btest-e2e\b' .github openshift-ci .prow ci scripts || true
🧹 Nitpick comments (1)
make/sandbox-ui.mk (1)
113-130: Consider fast-forward merge to match operator pairing semantics.Safer to fail if the plugin branch isn’t up-to-date.
- git --git-dir=${RHDH_PLUGINS_DIR}/.git --work-tree=${RHDH_PLUGINS_DIR} merge --allow-unrelated-histories --no-commit FETCH_HEAD + git --git-dir=${RHDH_PLUGINS_DIR}/.git --work-tree=${RHDH_PLUGINS_DIR} merge --ff-only FETCH_HEAD
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
build/sandbox-ui/Dockerfile(0 hunks)deploy/host-operator/ui-e2e-tests/toolchainconfig.yaml(1 hunks)make/sandbox-ui.mk(5 hunks)make/test.mk(1 hunks)openshift-ci/Dockerfile.tools(0 hunks)scripts/ci/manage-operator.sh(1 hunks)
💤 Files with no reviewable changes (2)
- build/sandbox-ui/Dockerfile
- openshift-ci/Dockerfile.tools
⏰ 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 (6)
scripts/ci/manage-operator.sh (1)
276-276: LGTM — newline only.deploy/host-operator/ui-e2e-tests/toolchainconfig.yaml (1)
19-21: Manually verify registration-service defaults and CRD validation for auth block removal
Ensure that omitting thespec.host.registrationService.authblock inToolchainConfigcauses the registration-service to default to the dev-sandbox SSO realm and that the CRD schema doesn’t require any of the removed fields.make/sandbox-ui.mk (4)
28-28: Deferring IMAGE_TO_PUSH_IN_QUAY after TAG is correct.
43-47: Envsubst wiring looks good.
135-136: Depth-1 clone is a good optimization.
145-148: CI images include Node (v24.3.0), Yarn (v1.22.22), and npx (v11.4.2); packaging step passes as-is.
| ifeq ($(GITHUB_ACTIONS),true) | ||
| @echo "using author ${AUTHOR}" | ||
| $(eval AUTHOR_LINK = https://github.com/${AUTHOR}) | ||
| @echo "detected branch ${BRANCH_NAME}" | ||
| # check if a branch with the same ref exists in the user's fork of rhdh-plugins repo | ||
| @echo "branches of ${AUTHOR_LINK}/rhdh-plugins - checking if there is a branch ${BRANCH_NAME} we could pair with." | ||
| curl ${AUTHOR_LINK}/rhdh-plugins.git/info/refs?service=git-upload-pack --output - | ||
| $(eval REMOTE_RHDH_PLUGINS_BRANCH := $(shell curl ${AUTHOR_LINK}/rhdh-plugins.git/info/refs?service=git-upload-pack --output - 2>/dev/null | grep -a "refs/heads/${BRANCH_NAME}$$" | awk '{print $$2}')) | ||
|
|
||
| # check if the branch with the same name exists, if so then merge it with master and use the merge branch, if not then use master | ||
| @echo "REMOTE_RHDH_PLUGINS_BRANCH: ${REMOTE_RHDH_PLUGINS_BRANCH}" | ||
| @$(MAKE) pair-if-needed REMOTE_RHDH_PLUGINS_BRANCH=${REMOTE_RHDH_PLUGINS_BRANCH} AUTHOR_LINK=${AUTHOR_LINK} | ||
| else | ||
| @echo "using rhdh-plugins repo from master" | ||
| @$(MAKE) clone-rhdh-plugins | ||
| endif | ||
| else | ||
| @echo "using local rhdh-plugins repo, no pairing needed: ${RHDH_PLUGINS_DIR}" | ||
| endif | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
GA pairing depends on undefined AUTHOR/BRANCH_NAME.
Default these from GA env to avoid no-op pairing.
-ifeq ($(GITHUB_ACTIONS),true)
+ifeq ($(GITHUB_ACTIONS),true)
+ # defaults for GA
+ AUTHOR ?= $(GITHUB_ACTOR)
+ BRANCH_NAME ?= $(GITHUB_HEAD_REF)
@echo "using author ${AUTHOR}"
$(eval AUTHOR_LINK = https://github.com/${AUTHOR})
@echo "detected branch ${BRANCH_NAME}"Committable suggestion skipped: line range outside the PR's diff.
|
/retest failed because the pairing pr redhat-developer/rhdh-plugins#1403 was not updated with master |
mfrancisc
left a comment
mfrancisc
left a comment
There was a problem hiding this comment.
Looks good 👍
Thanks for addressing my comments 🙏
| ifneq ($(strip $(REMOTE_RHDH_PLUGINS_BRANCH)),) | ||
| @echo "Branch ref of the user's fork to be used for pairing: \"${REMOTE_RHDH_PLUGINS_BRANCH}\"" | ||
| git config --global user.email "devsandbox@redhat.com" | ||
| git config --global user.name "KubeSaw" | ||
| # clone | ||
| rm -rf ${RHDH_PLUGINS_TMP} | ||
| git clone --depth=1 https://github.com/redhat-developer/rhdh-plugins.git ${RHDH_PLUGINS_TMP} | ||
| # add the user's fork as remote repo | ||
| git --git-dir=${RHDH_PLUGINS_TMP}/.git --work-tree=${RHDH_PLUGINS_TMP} remote add external ${AUTHOR_LINK}/rhdh-plugins.git | ||
| # fetch the branch | ||
| git --git-dir=${RHDH_PLUGINS_TMP}/.git --work-tree=${RHDH_PLUGINS_TMP} fetch external ${REMOTE_RHDH_PLUGINS_BRANCH} | ||
| # merge the branch with master | ||
| git --git-dir=${RHDH_PLUGINS_TMP}/.git --work-tree=${RHDH_PLUGINS_TMP} merge --allow-unrelated-histories --no-commit FETCH_HEAD | ||
| else |
There was a problem hiding this comment.
this code and the other ones look very similar (if not the same) to what we have in the scripts/ci. Probably stupid question - have you considered adding a file similar to manage-member-operator.sh and leverage the logic that is already available?
There was a problem hiding this comment.
never mind, I see the differences and challenges in the code
There was a problem hiding this comment.
TBH, I did this way to try to simplify as much as I could
| # merge the branch with master | ||
| git --git-dir=${RHDH_PLUGINS_TMP}/.git --work-tree=${RHDH_PLUGINS_TMP} merge --allow-unrelated-histories --no-commit FETCH_HEAD |
There was a problem hiding this comment.
one thing that I've just noticed - are you sure that this will work? If I understand it correction, this is executed before building & pushing the plugin image, but this would leave the repo without a commit (which could lead in dirty image tag or any other inconsistencies).
when you look at
toolchain-e2e/scripts/ci/manage-operator.sh
Lines 149 to 150 in 56fa2cf
Shouldn't we use this instead?
There was a problem hiding this comment.
@MatousJobanek , thank you so much!! I missed that detail, sorry! I will address the change
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
make/sandbox-ui.mk (1)
76-85: URL-encode login fields; add curl timeouts.Passwords with special characters can break form data; add timeouts to avoid CI hangs.
Apply this fix from the previous review comment:
- @status=$$(curl -s -o /dev/null -w "%{http_code}" \ + @status=$$(curl -s --connect-timeout 10 --max-time 30 -o /dev/null -w "%{http_code}" \ -X POST "https://sso.devsandbox.dev/auth/realms/sandbox-dev/protocol/openid-connect/token" \ - -d "grant_type=password" \ - -d "client_id=sandbox-public" \ - -d "username=$(SSO_USERNAME_READ)" \ - -d "password=$(SSO_PASSWORD_READ)"); \ + --data-urlencode "grant_type=password" \ + --data-urlencode "client_id=sandbox-public" \ + --data-urlencode "username=$(SSO_USERNAME_READ)" \ + --data-urlencode "password=$(SSO_PASSWORD_READ)"); \
🧹 Nitpick comments (1)
make/sandbox-ui.mk (1)
171-171: Update user cleanup to use the correct credential variable.Based on the past review comment, the user deletion should use the credential read variable for consistency.
- @oc delete usersignup ${SSO_USERNAME} -n ${HOST_NS} + @oc delete usersignup $(SSO_USERNAME_READ) -n ${HOST_NS}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
build/sandbox-ui/Dockerfile(0 hunks)make/sandbox-ui.mk(6 hunks)openshift-ci/Dockerfile.tools(0 hunks)
💤 Files with no reviewable changes (2)
- build/sandbox-ui/Dockerfile
- openshift-ci/Dockerfile.tools
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-05T11:01:26.390Z
Learnt from: rsoaresd
PR: codeready-toolchain/toolchain-e2e#1181
File: .github/workflows/publish-sandbox-ui-for-ui-e2e-tests.yml:70-71
Timestamp: 2025-08-05T11:01:26.390Z
Learning: For codeready-toolchain organization repositories, the team prefers to use master branch references for their internal toolchain-cicd actions rather than pinning to specific commit SHAs, prioritizing ease of maintenance and automatic updates over the additional security of version pinning.
Applied to files:
make/sandbox-ui.mk
🪛 GitHub Actions: publish-sandbox-ui-for-ui-e2e-tests
make/sandbox-ui.mk
[error] 142-142: Git merge failed during pairing: fatal: refusing to merge unrelated histories. Command: git --git-dir=rhdh-plugins/.git --work-tree=rhdh-plugins merge --ff-only FETCH_HEAD
[error] 120-120: Target 'get-rhdh-plugins' failed: exit code 2.
⏰ 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). (1)
- GitHub Check: Build & push operator bundles for e2e tests
🔇 Additional comments (2)
make/sandbox-ui.mk (2)
185-185: Remove the-count=1flag that was mentioned in the summary but update test execution appropriately.The AI summary indicates
-count=1was removed from e2e tests. The current implementation looks correct for most test execution patterns.Also applies to: 189-189
196-201: LGTM: Test targets properly updated for new credential handling.The test targets correctly pass the
ENVIRONMENTvariable and use the newRHDH_PLUGINS_TMPpath handling for both regular and local test execution.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
make/sandbox-ui.mk (4)
114-116: Don’t dump raw git protocol to logs; use git ls-remote.This avoids leaking info and simplifies parsing.
- curl ${AUTHOR_LINK}/rhdh-plugins.git/info/refs?service=git-upload-pack --output - - $(eval REMOTE_RHDH_PLUGINS_BRANCH := $(shell curl ${AUTHOR_LINK}/rhdh-plugins.git/info/refs?service=git-upload-pack --output - 2>/dev/null | grep -a "refs/heads/${BRANCH_NAME}$$" | awk '{print $$2}')) + @echo "Checking for branch ${BRANCH_NAME} in ${AUTHOR_LINK}/rhdh-plugins..." + $(eval REMOTE_RHDH_PLUGINS_BRANCH := $(shell git ls-remote -q --heads ${AUTHOR_LINK}/rhdh-plugins.git "refs/heads/${BRANCH_NAME}" 2>/dev/null | awk '{print $$2}'))
5-5: Fix TMPDIR join and guard destructive rm -rf.Current path resolves to “/tmprhdh-plugins” (or just “rhdh-plugins” when TMPDIR is empty), and rm -rf can wipe a local repo. Add a TMPDIR default, use a slash, update the equality check, and hard‑guard deletions to TMPDIR.
- RHDH_PLUGINS_TMP ?= $(TMPDIR)rhdh-plugins + TMPDIR ?= /tmp + RHDH_PLUGINS_TMP ?= $(TMPDIR)/rhdh-plugins @@ -ifeq ($(strip $(RHDH_PLUGINS_TMP)), $(TMPDIR)rhdh-plugins) +ifeq ($(strip $(RHDH_PLUGINS_TMP)), $(TMPDIR)/rhdh-plugins) @@ - rm -rf ${RHDH_PLUGINS_TMP}; \ + test -n "${RHDH_PLUGINS_TMP}" && \ + case "$$(realpath -m "${RHDH_PLUGINS_TMP}")" in /tmp/*) true ;; *) echo "Refusing to delete non-TMPDIR path: ${RHDH_PLUGINS_TMP}" >&2; exit 1;; esac; \ + rm -rf "${RHDH_PLUGINS_TMP}"; \Also applies to: 107-108, 149-151
13-24: TAG derivation: default PULL_NUMBER/SHAs and sanitize repo name.Avoid empty/“null” PR fragments and unsafe characters; make GA/PROW robust.
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}"; \ + REPOSITORY_NAME=$$(basename "$$GITHUB_REPOSITORY" | tr -c 'A-Za-z0-9._-/' '_'); \ + COMMIT_ID_SUFFIX=$$(printf "%s" "$${PULL_PULL_SHA:-$${GITHUB_SHA}}" | cut -c1-7); \ + PR_NUM=$${PULL_NUMBER:-$${GITHUB_REF_NAME:-$${GITHUB_RUN_NUMBER:-unknown}}}; \ + echo "from.$${REPOSITORY_NAME}.PR$${PR_NUM}.$${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}"; \ + REPO=$$(printf "%s" "$(REPO_NAME)" | tr -d '"' | tr -c 'A-Za-z0-9._-/' '_'); \ + PR_NUM=$${PULL_NUMBER:-unknown}; \ + echo "from.$${REPO}.PR$${PR_NUM}.$${COMMIT_ID_SUFFIX}"; \ fi; \Confirm REPO_NAME is always set when using CLONEREFS_OPTIONS; if not, derive it from CLONEREFS_OPTIONS (.refs[0].repo) similarly.
75-81: SSO login: URL‑encode fields and add timeouts.Prevents failures with special characters and avoids hangs.
- @status=$$(curl -s -o /dev/null -w "%{http_code}" \ + @status=$$(curl -sS --connect-timeout 10 --max-time 30 --retry 2 -o /dev/null -w "%{http_code}" \ -X POST "https://sso.devsandbox.dev/auth/realms/sandbox-dev/protocol/openid-connect/token" \ - -d "grant_type=password" \ - -d "client_id=sandbox-public" \ - -d "username=$(SSO_USERNAME_READ)" \ - -d "password=$(SSO_PASSWORD_READ)"); \ + --data-urlencode "grant_type=password" \ + --data-urlencode "client_id=sandbox-public" \ + --data-urlencode "username=$(SSO_USERNAME_READ)" \ + --data-urlencode "password=$(SSO_PASSWORD_READ)"); \
🧹 Nitpick comments (4)
make/sandbox-ui.mk (4)
34-34: oc get: remove conflicting output flags.Using both “--output=name” and “-o jsonpath” is invalid; keep jsonpath only.
-deploy-sandbox-ui: HOST_NS=$(shell oc get projects -l app=host-operator --output=name -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1) +deploy-sandbox-ui: HOST_NS=$(shell oc get projects -l app=host-operator -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1) @@ -clean-sandbox-ui: HOST_NS=$(shell oc get projects -l app=host-operator --output=name -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1) +clean-sandbox-ui: HOST_NS=$(shell oc get projects -l app=host-operator -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1) @@ -e2e-run-sandbox-ui: HOST_NS=$(shell oc get projects -l app=host-operator --output=name -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1) +e2e-run-sandbox-ui: HOST_NS=$(shell oc get projects -l app=host-operator -o jsonpath='{range .items[*]}{.metadata.name}{"\n"}{end}' | sort | tail -n 1)Also applies to: 166-166, 173-173
140-145: Pairing: handle non‑fast‑forward gracefully.If the fork branch isn’t up‑to‑date with master, fallback to master instead of failing the job.
- git --git-dir=${RHDH_PLUGINS_TMP}/.git --work-tree=${RHDH_PLUGINS_TMP} merge --ff-only FETCH_HEAD + if ! git --git-dir=${RHDH_PLUGINS_TMP}/.git --work-tree=${RHDH_PLUGINS_TMP} merge --ff-only FETCH_HEAD; then \ + echo "Pairing branch is not fast-forwardable; using master without pairing."; \ + fi
187-189: Quote secrets when generating .env and ignore missing cleanup.Avoids breakage when creds contain spaces/special chars; don’t fail if usersignup wasn’t created.
- @SSO_USERNAME=$(SSO_USERNAME_READ) SSO_PASSWORD=$(SSO_PASSWORD_READ) BASE_URL=${RHDH} BROWSER=firefox envsubst < deploy/sandbox-ui/ui-e2e-tests/.env > testsupport/sandbox-ui/.env + @SSO_USERNAME="$(SSO_USERNAME_READ)" SSO_PASSWORD="$(SSO_PASSWORD_READ)" BASE_URL="${RHDH}" BROWSER=firefox envsubst < deploy/sandbox-ui/ui-e2e-tests/.env > testsupport/sandbox-ui/.env @@ - @oc delete usersignup $(SSO_USERNAME_READ) -n $(HOST_NS) + @oc delete usersignup $(SSO_USERNAME_READ) -n $(HOST_NS) --ignore-not-found
88-93: Make OAuth secret creation idempotent.Creating an existing secret will fail reruns; apply instead.
- @oc create secret generic ${OPENID_SECRET_NAME} \ - --from-literal=clientSecret=dummy \ - --namespace=openshift-config + @oc -n openshift-config get secret ${OPENID_SECRET_NAME} >/dev/null 2>&1 || \ + oc create secret generic ${OPENID_SECRET_NAME} \ + --from-literal=clientSecret=dummy \ + --namespace=openshift-config
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
make/sandbox-ui.mk(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-05T11:01:26.390Z
Learnt from: rsoaresd
PR: codeready-toolchain/toolchain-e2e#1181
File: .github/workflows/publish-sandbox-ui-for-ui-e2e-tests.yml:70-71
Timestamp: 2025-08-05T11:01:26.390Z
Learning: For codeready-toolchain organization repositories, the team prefers to use master branch references for their internal toolchain-cicd actions rather than pinning to specific commit SHAs, prioritizing ease of maintenance and automatic updates over the additional security of version pinning.
Applied to files:
make/sandbox-ui.mk
⏰ 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: GolangCI Lint
- GitHub Check: Unit Tests
- GitHub Check: Build & push operator bundles for e2e tests
- GitHub Check: Build & push Developer Sandbox UI image for UI e2e tests
🔇 Additional comments (1)
make/sandbox-ui.mk (1)
30-31: Nit: compute IMAGE_TO_PUSH_IN_QUAY after validating TAG.Fine as-is; just confirming TAG is never empty after your fallbacks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
test/e2e/sandbox-ui/README.md (6)
17-20: Avoid putting SSO credentials on the CLI.Command‑line args leak via shell history and process lists. Prefer env file or interactive read. Example:
-`make test-ui-e2e SSO_USERNAME=${SSO_USERNAME} SSO_PASSWORD=${SSO_PASSWORD} PUSH_SANDBOX_IMAGE=true` +`set -a && source testsupport/sandbox-ui/.env && set +a && make test-ui-e2e PUSH_SANDBOX_IMAGE=true`-`make test-ui-e2e-local SSO_USERNAME=${SSO_USERNAME} SSO_PASSWORD=${SSO_PASSWORD} PUSH_SANDBOX_IMAGE=true` +`set -a && source testsupport/sandbox-ui/.env && set +a && make test-ui-e2e-local PUSH_SANDBOX_IMAGE=true`If Make supports it, document SSO_USERNAME_READ/SSO_PASSWORD_READ instead.
25-25: Define HOST_NS and show an example.Readers may not know what to pass. Add a note:
-`make test-sandbox-ui-in-container SSO_USERNAME=<SSO_USERNAME> SSO_PASSWORD=<SSO_PASSWORD> HOST_NS=<HOST_NS>` +`make test-sandbox-ui-in-container SSO_USERNAME=<SSO_USERNAME> SSO_PASSWORD=<SSO_PASSWORD> HOST_NS=<HOST_NS>` + +Where HOST_NS is the host-operator namespace (for example `toolchain-host-operator`). You can discover it with: +`oc get ns | rg host-operator`
29-40: ENVIRONMENT value looks wrong for stage/prod runs.Text says “against dev/stage/prod” but instructs ENVIRONMENT=dev. Recommend:
-- fill `testsupport/sandbox-ui/.env`, note that to run the UI E2E tests against dev/stage/prod, you need to set the ENVIRONMENT to dev. +- fill `testsupport/sandbox-ui/.env` and set ENVIRONMENT to one of: dev | stage | prod.-ENVIRONMENT=dev +ENVIRONMENT=<dev|stage|prod> +# Optionally adjust BASE_URL per environment if different from https://sandbox.redhat.com/
15-15: Fix heading level increments (MD001).Use h2 (##) after the h1.
-### Running UI E2E Tests locally +## Running UI E2E Tests locally -### Running UI E2E Tests in Container +## Running UI E2E Tests in Container -### Running UI E2E Tests against dev/stage/prod +## Running UI E2E Tests against dev/stage/prod -### Deploy Developer Sandbox UI in E2E mode +## Deploy Developer Sandbox UI in E2E mode -### Clean Developer Sandbox UI +## Clean Developer Sandbox UIAlso applies to: 23-23, 27-27, 46-46, 60-60
17-19: Document push auth requirements for PUSH_SANDBOX_IMAGE=true.Add a brief note that users must be logged into the target registry and OpenShift project before running these commands.
50-58: Automate insecure‑cert handling in tests (if possible).If Playwright config already sets ignoreHTTPSErrors for the routes, call it out; otherwise consider documenting a flag/workflow so manual pre‑browsing isn’t required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/e2e/sandbox-ui/README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
test/e2e/sandbox-ui/README.md
15-15: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
⏰ 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)
- GitHub Check: Unit Tests
- GitHub Check: Build & push operator bundles for e2e tests
- GitHub Check: Build & push Developer Sandbox UI image for UI e2e tests
🔇 Additional comments (1)
test/e2e/sandbox-ui/README.md (1)
13-13: prepare-and-deploy-e2e target exists and prepares UI E2E resources.make/test.mk defines prepare-and-deploy-e2e -> prepare-e2e + deploy-e2e (make/test.mk:61,84,92–93). deploy-e2e applies deploy/host-operator/ui-e2e-tests/toolchainconfig.yaml and deploys host/member operator manifests (which include RBAC and route resources).
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MatousJobanek, mfrancisc, 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 |
3 participants
Description
The UI E2E test environment is ready in the toolchain-e2e repository (check #1148). To integrate Developer Sandbox UI E2E tests into the toolchain-e2e CI pipeline, we need to implement the following:
(1) workflow for publishing the Developer Sandbox UI
(2) a new CI job in toolchain-e2e to execute the UI E2E tests.
This pr addresses (1), representing the initial phase of our environment separation strategy, where we'll split testing into two distinct environments: e2e-tests for toolchain resource e2e testing, and ui-e2e-tests for UI e2e testing. In subsequent phases, we plan to explore opportunities to consolidate these environments into one.
So, on this first phase, we will enable the UI E2E testing only on toolchain-e2e repo, making already available the pairing with host-operator, member-operator, registration-service and RHDH plugin repo. So, if:
there is host-operator PR and toolchain-e2e PR, we need to pair these two PRs so it uses:
there is RHDH-plugins PR and there is toolchain-e2e PR, we need to pair these two PRs so it uses:
Related PRs
#1181
codeready-toolchain/toolchain-cicd#149
Issue ticket number and link
SANDBOX-1371
Summary by CodeRabbit
Build
Tests
Chores
Configuration
Documentation