stage_executor: set avoidLookingCache only if mounting stage and not additional build context
#5693
Conversation
flouthoc
changed the title
avoidLookingCache only if mounting stage and not additional build context
|
@containers/buildah-maintainers PTAL |
|
LGTM |
This line looks like a typo in past commit as `additionalContext` which is found is not a local built stage so set `IsStage` to `false`. Signed-off-by: flouthoc <[email protected]>
set `avoidLookingCache` to `true` if `--mount` is using a freshly built stage and not for `additional-build-context`. Signed-off-by: flouthoc <[email protected]>
flouthoc
force-pushed
the
use-cache-with-build-context
branch
from
01ea275 to
d098893
Compare
|
Rebased. |
|
Hang on, if the contents of the additional build context are the same as, or changed from, on a previous run, do we, or do we not want to reuse the cache candidate? |
Following concern will be fixed with this #5691, where we will put the hash of the files in the context and make it part of the history.
I think cache stopped working for --mount when additional build context is used as source after PR #4526 Also I fixed the release note it should be |
|
@nalind PTAL |
Stopped working how? Did it fail to find a suitable image to use as a cache image? Did it choose an unsuitable image?
What behavior would someone have seen before when things went wrong? |
|
@flouthoc waiting on you? |
I don't know how I missed GH notification of this PR.
For this part the code which was introduced in PR #4526 accounted
Previously the layer based caching was simply not working for users who were using |
|
@nalind Could you PTAL again :) |
| _EOF | ||
|
|
||
| run_buildah build $WITH_POLICY_JSON --layers -t source2 -f $contextdir/Containerfile2 | ||
| expect_output --substring "firstfile" |
There was a problem hiding this comment.
This should probably also check that the string "Using cache" doesn't show up in the output for this build.
|
The "now works" in the release note still doesn't describe what was fixed. Are cached images used where they weren't before? Are they not when they shouldn't have been? Does cache evaluation now take into consideration something that it didn't before that it should have? If #5691 starts checking the contents of the bound directory, are we going to end up revisiting this change? |
|
I am thinking if this explains it more correctly, could you read this and see if is answering all questions correctly. Previously, Buildah would ignore cached layers for step
No, actually the reason of adding this change was that I found this bug while implementing #5691 since buildah was bypassing cache searching mechanism so my changes in #5691 were not working correctly, so I think this can be merged before #5691 so it actually use this change. |
|
@nalind Any thoughts on above comment. |
|
Just waiting for review on my last comment #5693 (comment) |
|
A friendly reminder that this PR had no activity for 30 days. |
|
LGTM |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
What this PR does / why we need it:
How to verify it
Newly added integration tests.
Which issue(s) this PR fixes:
Closes: #5692
Special notes for your reviewer:
Does this PR introduce a user-facing change?