Enhance Terminus Installation Reliability #32
Conversation
|
Download of both phar and checksum failing, flipping to draft until ready for re-review. |
| description: Disable session cache and force a new session to be initiated. | ||
| required: false | ||
| default: false | ||
| default: "false" |
There was a problem hiding this comment.
Is there a reason for switching this from boolean to string?
There was a problem hiding this comment.
I don't believe GHA really handle bool properly in these configs. IIRC, false is already a string, ^ just makes it more explicit.
| curl --silent \ | ||
| echo "Attempting to find latest Terminus version..." | ||
| # Try to get the latest release from GitHub API with proper error handling | ||
| if ! TERMINUS_RELEASE=$(curl --silent --fail --max-time 10 \ |
There was a problem hiding this comment.
Why constrain max-time to 10 seconds for everyone? Could this be made a configuration option rather than hard-coded?
| | perl -nle'print $& while m#"tag_name": "\K[^"]*#g' | ||
| ) | ||
| | perl -nle'print $& while m#"tag_name": "\K[^"]*#g'); then | ||
| echo "::error::Failed to connect to GitHub API to determine latest Terminus version." |
There was a problem hiding this comment.
This request could fail for a lot of reasons that weren't a connection failure; if there is any http response code at all then the connection was successful.
| ) | ||
| | perl -nle'print $& while m#"tag_name": "\K[^"]*#g'); then | ||
| echo "::error::Failed to connect to GitHub API to determine latest Terminus version." | ||
| echo "::error::Please specify a terminus-version input parameter for deterministic builds." |
There was a problem hiding this comment.
Again I don't think we should assert that all users should hard-code their terminus version. Some customers will want deterministic builds, other users will want to stay on the latest version.
| terminus-version: | ||
| description: | | ||
| The full version of Terminus to install. If omitted, the latest version is used. | ||
| For deterministic builds, specifying a version is recommended. |
There was a problem hiding this comment.
| For deterministic builds, specifying a version is recommended. | |
| Specifying a version provides deterministic results in exchange for not always having the latest version, with the caveat that deprecated versions will eventually be rejected by the the back-end. |
I like the idea of calling out specifying a version for deterministic results builds, but I don't think we want to make that a blanket recommendation for all users. Terminus evolves over time and we plan to begin hard-refusing requests from deprecated versions server-side in the near future.
Hard-coding a version would mean the customer would need to update their CI configuration from time to time. For larger enterprise customers, that's a reasonable tradeoff, but it's not a good tradeoff for everyone.
| echo "::error::Unable to determine a valid Terminus version from GitHub API." | ||
| echo "::error::Please specify a terminus-version input parameter for deterministic builds." |
There was a problem hiding this comment.
The current error handling buries any indication of what exactly went wrong. Yes, the current code suffers from that same problem, but while we're improving error handling we should also improve visibility into the details of the error. At minimum, if this condition fails, the value of $TERMINUS_RELEASE should be printed in the error message. Depending on what has gone wrong, $TERMINUS_RELEASE may be empty, and there are probably ways to get more detailed information fro curl about the nature of the problem.
| # Download checksum for verification | ||
| if curl --fail -L https://github.com/pantheon-systems/terminus/releases/download/$TERMINUS_RELEASE/terminus.phar.sha256 --output terminus.sha256; then | ||
| echo "Verifying download integrity..." | ||
| # Fix the checksum file format if needed (ensure it only contains hash and filename) | ||
| sed -i.bak 's/^.*\([0-9a-f]\{64\}\).*$/\1 terminus/g' terminus.sha256 | ||
| if ! sha256sum -c terminus.sha256; then | ||
| echo "::error::Checksum verification failed! The downloaded file may be corrupted." | ||
| exit 1 | ||
| fi | ||
| echo "Integrity verification passed." | ||
| else | ||
| echo "::warning::Could not download checksum file. Skipping integrity check." | ||
| fi |
There was a problem hiding this comment.
This checksum check should be removed, at least for now. That file doesn't exist, so currently this is just adding another request which is guaranteed to fail.
| # Basic file type verification | ||
| file_info=$(file terminus) | ||
| if ! echo "$file_info" | grep -q -E 'PHP script|executable|.+ASCII.+PHP'; then | ||
| echo "::error::Downloaded file is not a PHP script or executable. Got: $file_info" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
File type check should be removed. The output of the file command is not consistent across systems, and it's clear you've already encountered this since the grep is attempting to match 3 different possible responses.
It is receiving a different response in Github Actions than what the pattern match currently covers. Adding more conditions to account for this additional format will still not provide full coverage. Additionally it assumes that the file command exists, which will not always be the case.
This check is also made redundant by the terminus --version test at the end. That test will not succeed if what's downloaded isn't the correct type.
|
@lcatlett what would you think of moving the GitHub Action that installs Terminus to the Terminus repo? Here's an example of from Deno where the repo containing the command line tool also contains the Action to install that version: https://github.com/denoland/deployctl |
Problem
The current Terminus installation process has several reliability issues:
These issues have affected a few Pantheon customers, one of which recently encountered hard-to-diagnose failures in their CI pipelines. Errors such as
/usr/local/bin/terminus: line 1: Not: command not foundwere observed, indicating a download failure while the workflow proceeded, causing significant workflow disruptions and troubleshooting challenges.Solution
This pull request addresses these issues by:
curl --fail.terminus --version.Implementation Details
1. Improved Version Selection
These modifications enhance the reliability of the action and mitigate common installation failures while preserving compatibility with existing workflows that explicitly specify a version.
Code Changes
The pull request updates the "Set Terminus version" and "Install Terminus" steps in action.yml to incorporate these enhancements:
Users who explicitly specify a version using the
terminus-versionparameter will continue to experience the same behavior, but the reliability of the action has been significantly improved. Users who rely on automatic version detection will benefit from enhanced error handling and clear failure messages, providing more timely feedback in the event of installation issues.These modifications directly address the challenges faced by Pantheon customers running into CI failures with errors like
line 1: Not: command not foundby promptly identifying and resolving installation problems as the root cause, rather than allowing them to recur later with cryptic error messages that can hinder development velocity and consume troubleshooting time for both customers and support teams trying to diagnose issues originating in the CI configuration.