Added optional HTTP basic auth credentials to Diffy visual regression workflow.#2536
Conversation
WalkthroughThis PR adds optional HTTP basic authentication to the Diffy visual regression workflow by introducing four new GitHub Actions secrets for source/target credentials and conditionally passing them to the ChangesVisual Regression Authentication
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2536 +/- ##
==========================================
- Coverage 86.56% 86.11% -0.46%
==========================================
Files 94 87 -7
Lines 4660 4501 -159
Branches 47 3 -44
==========================================
- Hits 4034 3876 -158
+ Misses 626 625 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/test-vr.yml:
- Around line 186-191: The current shell snippet silently ignores partial
credential pairs and should instead fail fast: add guards around the
VR_SOURCE_HTTP_USER/VR_SOURCE_HTTP_PASS pair and the
VR_TARGET_HTTP_USER/VR_TARGET_HTTP_PASS pair to detect when only one of the
variables is set and exit with a clear error; keep the existing behavior of
appending to auth_args only when both variables are non-empty (the existing
auth_args+=(...) logic), but before that check use tests like -n for one and -z
for the other (or combine tests) and call echo >&2 with a descriptive message
and exit 1 if a half-configured pair is found so the workflow fails early and
clearly.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cd0e4934-3fca-43e6-aee5-2074221411e7
⛔ Files ignored due to path filters (1)
.vortex/installer/tests/Fixtures/handler_process/visual_regression_enabled/.github/workflows/test-vr.ymlis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (1)
.github/workflows/test-vr.yml
| auth_args=() | ||
| if [ -n "${VR_SOURCE_HTTP_USER}" ] && [ -n "${VR_SOURCE_HTTP_PASS}" ]; then | ||
| auth_args+=(--env1User="${VR_SOURCE_HTTP_USER}" --env1Pass="${VR_SOURCE_HTTP_PASS}") | ||
| fi | ||
| if [ -n "${VR_TARGET_HTTP_USER}" ] && [ -n "${VR_TARGET_HTTP_PASS}" ]; then | ||
| auth_args+=(--env2User="${VR_TARGET_HTTP_USER}" --env2Pass="${VR_TARGET_HTTP_PASS}") |
There was a problem hiding this comment.
Fail fast on half-configured credential pairs.
If only one of the *_HTTP_USER / *_HTTP_PASS secrets is set, this step silently drops auth and turns a secrets misconfiguration into a later Diffy failure. Reject partial pairs here so the workflow fails with a clear error.
Suggested guard
auth_args=()
+ if { [ -n "${VR_SOURCE_HTTP_USER}" ] || [ -n "${VR_SOURCE_HTTP_PASS}" ]; } &&
+ { [ -z "${VR_SOURCE_HTTP_USER}" ] || [ -z "${VR_SOURCE_HTTP_PASS}" ]; }; then
+ echo "::error::VR_SOURCE_HTTP_USER and VR_SOURCE_HTTP_PASS must both be set or both be unset."
+ exit 1
+ fi
+ if { [ -n "${VR_TARGET_HTTP_USER}" ] || [ -n "${VR_TARGET_HTTP_PASS}" ]; } &&
+ { [ -z "${VR_TARGET_HTTP_USER}" ] || [ -z "${VR_TARGET_HTTP_PASS}" ]; }; then
+ echo "::error::VR_TARGET_HTTP_USER and VR_TARGET_HTTP_PASS must both be set or both be unset."
+ exit 1
+ fi
if [ -n "${VR_SOURCE_HTTP_USER}" ] && [ -n "${VR_SOURCE_HTTP_PASS}" ]; then
auth_args+=(--env1User="${VR_SOURCE_HTTP_USER}" --env1Pass="${VR_SOURCE_HTTP_PASS}")
fi
if [ -n "${VR_TARGET_HTTP_USER}" ] && [ -n "${VR_TARGET_HTTP_PASS}" ]; then
auth_args+=(--env2User="${VR_TARGET_HTTP_USER}" --env2Pass="${VR_TARGET_HTTP_PASS}")🤖 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/test-vr.yml around lines 186 - 191, The current shell
snippet silently ignores partial credential pairs and should instead fail fast:
add guards around the VR_SOURCE_HTTP_USER/VR_SOURCE_HTTP_PASS pair and the
VR_TARGET_HTTP_USER/VR_TARGET_HTTP_PASS pair to detect when only one of the
variables is set and exit with a clear error; keep the existing behavior of
appending to auth_args only when both variables are non-empty (the existing
auth_args+=(...) logic), but before that check use tests like -n for one and -z
for the other (or combine tests) and call echo >&2 with a descriptive message
and exit 1 if a half-configured pair is found so the workflow fails early and
clearly.
|
Code coverage (threshold: 90%) Per-class coverage |
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
|
Code coverage (threshold: 90%) Per-class coverage |
Summary
The Diffy
project:compareCLI command supports--env1User/--env1Passand--env2User/--env2Passflags for HTTP basic auth, but they must only be passed when both the user and password are non-empty. The upstream PHP SDK usesisset()to check for presence - an empty string passes that check and causes Diffy to attempt basic-auth with empty credentials, which then fails against servers protected by Acquia Shield orhtpasswd. This change conditionally builds anauth_argsbash array and splices it into thediffy.phar project:comparecall only when both halves of a credential pair are present.Four optional GitHub Actions secrets (
VR_SOURCE_HTTP_USER,VR_SOURCE_HTTP_PASS,VR_TARGET_HTTP_USER,VR_TARGET_HTTP_PASS) are wired into the comparison step. When unset (the common case for public environments) theauth_argsarray stays empty and the call is identical to the previous behaviour. The docs and installer fixture are updated to match.Changes
.github/workflows/test-vr.yml- Added four optionalenvbindings for the new secrets; introduced anauth_args=()bash array that is populated only when both halves of a credential pair are non-empty; merged the array into thediffy.phar project:comparecall using the${arr[@]+"${arr[@]}"}safe-expansion idiom (no-op when the array is empty, safe underset -u)..vortex/installer/tests/Fixtures/handler_process/visual_regression_enabled/.github/workflows/test-vr.yml- Regenerated fixture to match the template above..vortex/docs/content/development/visual-regression.mdx- Documented the four new optional secrets in the credentials table and added a paragraph explaining the conditional-passing behaviour and when the secrets are needed..vortex/docs/cspell.json- Addedhtpasswdto the spell-check dictionary.Before / After
Summary by CodeRabbit
New Features
Documentation