remotecache: fix missing CheckDescriptor method #4771
Merged
Conversation
This file contains 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
This method is needed by GHA cache backend to detect if Github deletes blobs from the cache because of storage caps. It got missing when Info() support was added. Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
ktock
approved these changes
Mar 18, 2024
|
LGTM about this patch.
For avoiding interface detection, maybe we should define the type that explicitly defines all necessary methods and use it for |
vvoland
approved these changes
Mar 18, 2024
|
Confirming that we do use that cache: https://github.com/dagster-io/dagster-cloud-action/blob/main/actions/serverless_prod_deploy/action.yml#L80 |
crazy-max
approved these changes
Mar 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
fixes #4765
This method is needed by GHA cache backend to detect if Github deletes blobs from the cache because of
storage caps. It went missing when Info() support was added in #4558 for the same issue as #4558 (comment) . Later #4694 added back stargz method but not
CheckDescriptor.Overall we need to find a safer approach to this, especially because it is impossible to test because of being controlled by Github's storage. For this case, I think
CheckDescriptoris not really needed anymore but can be replaced withInfo()as commented in #4558 (comment) . But the stragz methods should not rely on interface detection either. The current PR is the minimal change that we can pick to v0.13 branch.@vvoland @ktock
@gibsondan and others, please confirm your build was importing cache from GHA.