Skip to content

fix: add fallback to master for test-controller action #109

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
probelabs wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: add fallback to master for test-controller action #109

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e39bada
fix: add fallback to master for test-controller action
buger Apr 9, 2026
ae368f6
fix: implement client-side fallback for TUI configuration fetching ba…
buger Apr 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions .github/actions/tests/test-controller/action.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,30 @@
TRIGGER: ${{ github.event_name }}
run: |
set -eo pipefail
curl -s --retry 5 --retry-delay 10 --fail-with-body "http://tui.internal.dev.tyk.technology/v2/$VARIATION/$REPO_NAME/$BASE_REF/$TRIGGER/$TEST_TYPE.gho" | tee -a "$GITHUB_OUTPUT"

Check warning on line 38 in .github/actions/tests/test-controller/action.yaml

View check run for this annotation

probelabs / Visor: quality

logic Issue 

The new configuration fetching and fallback logic within the shell script is not covered by automated tests. While manual verification is mentioned, the lack of automated tests creates a risk of regressions if this critical script is modified in the future.
Raw output 
Consider adding automated tests for this action's script logic. Tools like `bats-core` can be used to test shell scripts by mocking the `curl` command and verifying that the fallback logic behaves correctly for different HTTP status codes (e.g., 200, 404, 500).
URL="http://tui.internal.dev.tyk.technology/v2/$VARIATION/$REPO_NAME/$BASE_REF/$TRIGGER/$TEST_TYPE.gho"

# Fetch the configuration and capture the HTTP status code

Check warning on line 41 in .github/actions/tests/test-controller/action.yaml

View check run for this annotation

probelabs / Visor: quality

style Issue 

The URL for fetching the test configuration is constructed in two separate places, with significant duplication. This violates the Don't Repeat Yourself (DRY) principle, making the script harder to read and increasing the chance of errors during future modifications.
Raw output 
Refactor the script to build the URL from common base components. This will remove duplication and make the logic clearer.
HTTP_CODE=$(curl -s -o response.txt -w "%{http_code}" --retry 5 --retry-delay 10 "$URL")

# If 404, fallback to master branch

Check warning on line 44 in .github/actions/tests/test-controller/action.yaml

View check run for this annotation

probelabs / Visor: quality

style Issue 

The script creates a temporary file named `response.txt` but does not remove it upon completion. In ephemeral environments like GitHub Actions runners this is less critical, but it is a best practice to clean up temporary files to avoid potential side-effects.
Raw output 
Use `mktemp` to create a uniquely named temporary file and a `trap` command to ensure its removal when the script exits, regardless of success or failure. This makes the script more robust and cleaner.
if [ "$HTTP_CODE" -eq 404 ]; then

Check failure on line 45 in .github/actions/tests/test-controller/action.yaml

View check run for this annotation

probelabs / Visor: security

security Issue 

The action fetches configuration from `http://tui.internal.dev.tyk.technology` over an unencrypted connection. This exposes the contents of the configuration file to network eavesdropping. If the configuration contains secrets or sensitive data, they could be compromised.
Raw output 
Change the protocol from `http` to `https` in the URLs to ensure the connection is encrypted. The internal server must be configured to support HTTPS.

Check failure on line 45 in .github/actions/tests/test-controller/action.yaml

View check run for this annotation

probelabs / Visor: security

security Issue 

The URL path is constructed by concatenating variables like `$BASE_REF`, `$VARIATION`, and `$TEST_TYPE` without any sanitization. A malicious branch name or action input containing path traversal sequences (e.g., `../`) could allow an attacker to request arbitrary files from the `tui.internal.dev.tyk.technology` server, assuming the server is also vulnerable.
Raw output 
Before constructing the URL, validate each variable used as a path segment to ensure it does not contain `..` or other characters that could be used for path manipulation. Add validation checks at the beginning of the script.
echo "Configuration not found for base_ref '$BASE_REF'. Falling back to 'master' branch."
FALLBACK_URL="http://tui.internal.dev.tyk.technology/v2/$VARIATION/$REPO_NAME/master/$TRIGGER/$TEST_TYPE.gho"
HTTP_CODE=$(curl -s -o response.txt -w "%{http_code}" --retry 5 --retry-delay 10 "$FALLBACK_URL")
fi

Check warning on line 49 in .github/actions/tests/test-controller/action.yaml

View check run for this annotation

probelabs / Visor: quality

architecture Issue 

The fallback branch is hardcoded to 'master'. This reduces the action's reusability and may cause failures in repositories that use a different default branch name, such as 'main'.
Raw output 
To make the action more robust, the default branch name should not be hardcoded. Consider passing the repository's default branch as an input to the action, which can be supplied by the calling workflow using `github.repository.default_branch`.

# If still an error (>= 400), fail the step and output the body

Check warning on line 51 in .github/actions/tests/test-controller/action.yaml

View check run for this annotation

probelabs / Visor: security

security Issue 

When a `curl` request fails (HTTP status >= 400), the entire response body is printed to the GitHub Actions log via `cat response.txt`. If the server's error pages contain sensitive information such as stack traces, internal file paths, or configuration details, this information will be exposed in the logs.
Raw output 
Avoid printing the full response body on error. If debugging information is needed, consider using GitHub secrets to enable a verbose mode or ensure the server is configured to return generic, non-revealing error pages. Alternatively, you could truncate the output, for example: `head -c 1024 response.txt`.
if [ "$HTTP_CODE" -ge 400 ]; then
echo "Failed to fetch configuration: HTTP $HTTP_CODE"
cat response.txt

Check warning on line 54 in .github/actions/tests/test-controller/action.yaml

View check run for this annotation

probelabs / Visor: architecture

architecture Issue 

The fallback branch is hardcoded to `master`. This creates a tight coupling with an implicit convention of the external configuration service. If this convention changes, or if the action is used in a context that requires a different fallback branch (e.g., `main`), the action will require code modification. This reduces its reusability and maintainability.
Raw output 
To improve flexibility and make the dependency explicit, introduce a new action input for the fallback branch, with `master` as its default value. This allows callers to override the fallback branch if needed.
exit 1

Check warning on line 55 in .github/actions/tests/test-controller/action.yaml

View check run for this annotation

probelabs / Visor: architecture

architecture Issue 

The `curl` command for fetching the configuration is duplicated for both the initial attempt (line 49) and the fallback attempt (line 55). This duplication makes the script harder to read and maintain. Any changes to the `curl` options (e.g., timeouts, retry logic) would need to be applied in two places.
Raw output 
Refactor the fetching logic into a reusable shell function within the script. The function could take the branch name as a parameter, construct the URL, execute `curl`, and return the status code. This would eliminate code duplication and centralize the fetching logic.
fi

# Write the successful response to GITHUB_OUTPUT
cat response.txt | tee -a "$GITHUB_OUTPUT"

Check warning on line 59 in .github/actions/tests/test-controller/action.yaml

View check run for this annotation

probelabs / Visor: performance

performance Issue 

The script uses a hardcoded temporary file name (`response.txt`) to store the curl response. This is not ideal for resource management as the file is not cleaned up after the script finishes. In an ephemeral CI runner this is a minor issue, but it's a best practice to manage temporary files explicitly.
Raw output 
Use `mktemp` to create a temporary file with a unique name and a `trap` to ensure it is removed when the script exits, regardless of success or failure. This improves the script's robustness and resource handling.

if ! [[ $VARIATION =~ prod ]] ;then
echo "::warning file=.github/workflows/release.yml,line=24,col=1,endColumn=8::Using non-prod variation"
echo "### :warning: You are using VARIATION=${VARIATION} in test-controller-{{ .test }}" >> $GITHUB_STEP_SUMMARY
echo "### :warning: You are using VARIATION=${VARIATION} in test-controller-" >> $GITHUB_STEP_SUMMARY
fi
Loading