Fix Cabal smoke-test workflow #5936
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The changes to support commenting on a PR broke the ability to manually run the workflow. This condionalizes the parts required for `pull_request_target` so that running on a branch explicitly will work. It also conditionalizes the `continue-on-error` value so that runs triggered by `workflow_dispatch` will fail (if something goes wrong) rather than hiding the failure as we do when we add a comment to the PR.
It was previously only based on the cabal.project.freeze file, which changes very rarely. However, a change to build flags (as happened in unisonweb#5856) can cause dependencies to need to be rebuilt. But if the cache key was a perfect match, we wouldn’t cache the updated dependency builds. This adds a hash for all Cabal files to the key, so any change that would affect dependencies causes a cache miss. It also adds the old key (which is a prefix of the new one) as a restore key, so even when cabal files change, we load a cache for the correct LTS (GHC version, etc.) which can prevent cache churn in the period after LTS updates.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
There are two issues fixed here:
workflow_dispatchtrigger didn’t work because the workflow was trying to look up the PR number, which doesn’t exist forworkflow_dispatchandImplementation notes
This also makes the workflow behave a bit better under
workflow_dispatchthan it would have – with a PR, we let the job pass on failure, and add a comment to the PR. Sinceworkflow_dispatchdoesn’t give us a place to comment, we let the job fail on failure.Test coverage
The modified workflow won’t run on this repo until it’s merged, so I’ve run the updated workflow on my fork. This at least lets us know that there are no syntax errors, and we get a successful run after triggering with
workflow_dispatch.Unfortunately, my fork doesn’t have a cache for this, so there’s a cache miss – but the keys look correct in the output, and once run on this repo, we should expect to see a cache miss, with the cache restored from the freeze-file-hashed prefix, and then the cache updated with the new dependency builds.