-
Notifications
You must be signed in to change notification settings - Fork 1
BUILD-11964: Harden config-uv dependency cache keys #320
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
base: master
Are you sure you want to change the base?
Changes from all commits
9b71829
1d8cff6
15ca28e
22f96ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,9 @@ inputs: | |
| uv-cache-dir: | ||
| description: Path to the uv cache directory, relative to GitHub workspace | ||
| default: .cache/uv | ||
| cache-dependency-glob: | ||
| description: Glob of dependency files for jobs that install more than one uv project | ||
| default: '' | ||
| disable-caching: | ||
| description: Whether to disable uv caching entirely | ||
| default: 'false' | ||
|
|
@@ -88,13 +91,41 @@ runs: | |
| run: | | ||
| echo "ARTIFACTORY_READER_ROLE=${ARTIFACTORY_READER_ROLE}" >> "$GITHUB_ENV" | ||
|
|
||
| - name: Resolve uv cache key | ||
| id: uv-cache-key | ||
| if: steps.config-uv-completed.outputs.skip != 'true' && inputs.disable-caching != 'true' | ||
| shell: bash | ||
| env: | ||
| CACHE_DEPENDENCY_GLOB: ${{ inputs.cache-dependency-glob }} | ||
| DIRECTORY_DEPENDENCY_HASH: | ||
| ${{ hashFiles(format('{0}/uv.lock', inputs.working-directory), | ||
| format('{0}/pyproject.toml', inputs.working-directory)) }} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why pyproject.toml? Maybe a question for @hedinasr since it was in "Cache uv dependencies". |
||
| GLOB_DEPENDENCY_HASH: ${{ hashFiles(inputs.cache-dependency-glob) }} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rename GLOB_DEPENDENCY_HASH -> CACHE_DEPENDENCY_HASH |
||
| WORKING_DIRECTORY: ${{ inputs.working-directory }} | ||
| run: | | ||
| dependency_hash="$DIRECTORY_DEPENDENCY_HASH" | ||
| dependency_source="working-directory '${WORKING_DIRECTORY}'" | ||
| if [[ -n "$CACHE_DEPENDENCY_GLOB" ]]; then | ||
|
Comment on lines
+94
to
+108
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Quality: New cache-key resolution bash logic has no test coverageThe 'Resolve uv cache key' step adds non-trivial branching (glob-vs-working-directory hash selection and the empty-hash 'disable caching + warn' path), but spec/config-uv_spec.sh only exercises uv_config.sh, not the composite action's embedded bash. The PR test plan explicitly notes the shell specs are unaffected, so these branches ship untested. Consider extracting the resolution logic into a sourced script that shellspec can cover, or adding a targeted test for the empty-hash and glob paths. Was this helpful? React with 👍 / 👎 |
||
| dependency_hash="$GLOB_DEPENDENCY_HASH" | ||
| dependency_source="cache-dependency-glob '${CACHE_DEPENDENCY_GLOB}'" | ||
| fi | ||
|
Comment on lines
+106
to
+111
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Edge Case: cache-dependency-glob silently overrides working-directory hashWhen both Was this helpful? React with 👍 / 👎 |
||
| if [[ -z "$dependency_hash" ]]; then | ||
| message="No dependency files matched ${dependency_source}. " | ||
| message+="Point config-uv at the uv project or dependency files that the job installs." | ||
| echo "::warning title=uv caching disabled::$message" | ||
| echo "cache_enabled=false" >> "$GITHUB_OUTPUT" | ||
| exit 0 | ||
| fi | ||
| echo "cache_enabled=true" >> "$GITHUB_OUTPUT" | ||
| echo "dependency_hash=$dependency_hash" >> "$GITHUB_OUTPUT" | ||
|
|
||
| - name: Cache uv dependencies | ||
| uses: SonarSource/gh-action_cache@4e40632e780e11a8bbe9b721985ab22b42847cc4 # v1.7.2 | ||
| if: steps.config-uv-completed.outputs.skip != 'true' && inputs.disable-caching != 'true' | ||
| if: steps.config-uv-completed.outputs.skip != 'true' && inputs.disable-caching != 'true' && | ||
| steps.uv-cache-key.outputs.cache_enabled == 'true' | ||
| with: | ||
| path: ${{ github.workspace }}/${{ inputs.uv-cache-dir }} | ||
| key: uv-${{ runner.os }}-${{ hashFiles(format('{0}/uv.lock', inputs.working-directory), | ||
| format('{0}/pyproject.toml', inputs.working-directory)) }} | ||
| key: uv-${{ runner.os }}-${{ steps.uv-cache-key.outputs.dependency_hash }} | ||
| restore-keys: uv-${{ runner.os }}- | ||
|
|
||
| - uses: jdx/mise-action@e6a8b3978addb5a52f2b4cd9d91eafa7f0ab959d # v4.2.0 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add to the README. Point in the PR description to the usages.
Isn't the default "inputs.working-directory/uv.lock"?
Why do we need this instead of using the working-directory?