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
Multiple directories and path glob pattern support for pipeline caching #2834
Conversation
Users/etdenn/minimatch cache path
details = matchedDirectories.Values.ToArray(); | ||
resolvedSegments.AddRange(matchedDirectories.Values); | ||
|
||
// TODO: Is it the right behavior to throw an exception if a path segment isn't resolveable? |
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.
I'd love the team's input on what this behavior should be. For any given path segment, if there are no matches, should the tasks fail or simply not include it?
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.
hmm, We should fail imo
} | ||
return uploadPath; | ||
// TODO: what is the right way to handle !ContentFormat.SingleTar |
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.
I'm not quite clear on when the ContentFormat
will be something other than SingleTar
so I could use some guidance on how to handle that case here.
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.
ContentFormat would be 'Files' for older Cache entries.
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.
Are 'Files' input a single item? How do I test backwards compatibility with ContentFormat.Files
entries?
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.
So, initially we didn't had tarring, and hence for the cache entries before tarring, we had the input as 'Files'. Your approach LGTM.
@fadnavistanmay should the For reference, GitHub Actions includes the |
This PR addresses microsoft/azure-pipelines-tasks#11219 |
} | ||
else | ||
{ | ||
DownloadDedupManifestArtifactOptions options = DownloadDedupManifestArtifactOptions.CreateWithManifestId( | ||
manifestId, | ||
targetDirectory, | ||
pathSegments[0], // TODO: is this the right format here |
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.
Looks right.
Thanks for the contribution Ethan. LGTM - @b-barthel - do you mind being the second pair of eyes for this. Thanks |
@fadnavistanmay / @b-barthel what are the next steps for this PR? My main concern is still with backwards compatibility with existing caches.. |
Edit: just realized that this is not released yet 😒 =============== @ethanis the following doesn't seem to work on Mac agent:
Because I am trying the following task in my mono repo (using yarn workspace):
result in the following error:
I have also tried using the env variable AZP_CACHING_CONTENT_FORMAT=Files like:
but in this case it result in the following error:
Do you have any idea how to make it work? |
@fadnavistanmay: do you have any rough estimates on timeline for getting this PR completed for @GoMino? |
Any further updates on this PR? |
This seems to be an interesting PR. Does this have any performance benefits compared to using separate tasks? |
Hi folks. I strongly support adding this feature. This is definitely a huge feature missing compared to the unofficial/V1 cache task. Without a feature like this PR is adding, I prefer the old cache task. Having to cache three different Our Would be great to have that again here on the official/V2 task. Thanks to the author for posting, and to the maintainers for considering this PR. |
@ethanis / @mjroghelia - Let's get this PR in. |
Hi again, folks. Any update on this? Thanks. |
@fadnavistanmay sorry, I missed your earlier approval of this. @ethanis If you rebase this on master and remove the TODO comments with questions that were addressed in @fadnavistanmay's earlier review I will merge it. |
@mjroghelia @fadnavistanmay so excited to get this shipped! |
Not sure what's going on with this failing Functional Test - it's coming from a project not touched by this PR.. |
This extends the pipeline caching tasks so that multiple paths can be cached with a single task. The following are supported scenarios:
Pipeline.Workspace
.System.DefaultWorkingDirectory
.Pipeline.Workspace
.Pipeline.Workspace
The following are unsupported scenarios:
Pipeline.Workspace
Pipeline.Workspace
The limitation of having multiple paths being rooted in
Pipeline.Workspace
is because the tarball needs to be created somewhere, andPipeline.Workspace
seemed like the most build agent agnostic location to anchor on.There will need to be a PR to the azure-pipelines-tasks repository to update the docs for the task inputs when/if these changes make it in.
Finally, here a some examples:
The following will fail because 1 path segment is outside
Pipeline.Workspace
and 1 is within