Skip to content

Fix faulty error output on empty cache #30

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.

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

Closed

Conversation

kyletaylored
Copy link
Contributor

When there's no existing cache, we skip the step with an exit 1, but that also produces an error output for the step. Instead, this will exit gracefully (exit 0) by skipping the step and continuing to the next step to authenticate.

fixes #29

@kyletaylored kyletaylored requested a review from a team as a code owner November 2, 2024 03:45
@kyletaylored
Copy link
Contributor Author

Ok, this works a little more gracefully and I was actually able to test it. Instead of relying on exit codes, we can just explicitly set a step output and not throw any errors. If we can validate everything, then skip authenticating, else continue to authenticate.

@DarkteK
Copy link

DarkteK commented Apr 10, 2025

Do we have a date on when this can be merge? It's been more than 5 months and we are still waiting for this fix/feature to be integrated in the master branch please @pwtyler @namespacebrian

jazzsequence
jazzsequence previously approved these changes Apr 11, 2025
@stevector
Copy link

I'm trying this PR in pantheon-systems/push-to-pantheon#38 and it seems to resolve the "exit 1" problem, but as I look for evidence of caching across jobs, it seems like it is not actually caching across jobs. Here is output from a 2nd serial job:
Screenshot 2025-04-11 at 2 36 02 PM

Comment on lines +87 to 94
# Check for restored user (string) or allow command to fail gracefully.
TERMINUS_USER=$(terminus auth:whoami || true)

# Check TERMINUS_USER for string.
if [ -z "$TERMINUS_USER" ]; then
echo "No valid session found. "
exit 1
exit 0
fi
Copy link
Member

@pwtyler pwtyler Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

        # Check for restored user (string) or allow command to fail gracefully.
        TERMINUS_USER=""
        if ! TERMINUS_USER=$(terminus auth:whoami); then
          echo "terminus auth:whoami failed: ${TERMINUS_USER}"
          echo "No valid session found."
          exit 0
        fi

        # Also assert TERMINUS_USER is a string.
        if [ -z "$TERMINUS_USER" ]; then
          echo "No valid session found. "
          exit 0
        fi

the or true inside the subshell smells to me, although I can't actually rationalize to myself why it's not fine. Setting TERMINUS_USER before does prevent the assignment from masking the exit code.


# Verify that the encrypted session file was restored from cache.
test -s ${{ steps.configure-cache.outputs.path }}
if [ ! -s "${{ steps.configure-cache.outputs.path }}" ]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if [ ! -s "${{ steps.configure-cache.outputs.path }}" ]; then
if [[ ! -s "${{ steps.configure-cache.outputs.path }}" ]]; then

we're in bash, use bash.

@jazzsequence jazzsequence dismissed their stale review April 11, 2025 20:17

|| true is not a good solution because it will assign the variable to true

@namespacebrian
Copy link
Contributor

To be fully confident, I would want to have a workflow that used this and hit the rate limit without adding the caching stuff around it, and then didn't it the rate limit once that scaffolding was added.

If we could get a customer who uses the caching to avoid the rate limit to test this, that'd be enough, but I'm not sure we have a eager guinea pig.

We could create 2 workflows on this repo, one that doesn't use the caching and one that does..

@pwtyler
Copy link
Member

pwtyler commented Apr 25, 2025

Applied with 8e024bd merging #37.

@pwtyler pwtyler closed this Apr 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Workflow summary shows failure when the Terminus token is uncached
6 participants