replace resolveimageconfig with generic sourcemetaresolver #4563
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
tonistiigi
force-pushed
the
sourceresolver
branch
2 times, most recently
from
c76c772
to
6a13c1f
Compare
This was unfortuantely just wrong. The reference for a named context is is fetched using `nameWithPlatform`. When a source policy mutated the ref this was recursively trying to resolve the reference again, however it was using the mutated ref as the new context name (and even that was not correct because `name` was made the new `nameWithPlatform` which is what is actually used as the key rather than `name`). This shouldn't be mutating the context key but rather the ref the context is pointing to. This change updates the build opts such the value pointed at by `nameWithPlatform` in the build opts map points to the updated ref instead of the original. Signed-off-by: Brian Goff <[email protected]>
tonistiigi
force-pushed
the
sourceresolver
branch
8 times, most recently
from
76c8b86
to
aaf5d86
Compare
tonistiigi
force-pushed
the
sourceresolver
branch
from
aaf5d86
to
9e14155
Compare
|
@cpuguy83 This is green now. I have follow-ups coming that change the Dockerfile to use new method and add the possibility for sourcepolicy to switch from non-image to image but would be good to get the base in on its own so it doesn't grow too large. |
cpuguy83
reviewed
Feb 10, 2024
| @@ -74,15 +74,13 @@ func (e *Engine) Evaluate(ctx context.Context, op *pb.Op) (bool, error) { | |||
| return mutated, errors.Wrapf(ErrTooManyOps, "too many mutations on a single source") | |||
| } | |||
|
|
|||
| srcOp := op.GetSource() | |||
| if srcOp == nil { | |||
There was a problem hiding this comment.
I noticed we are passing op.GetSoruce() directly into Evaluate.
Do we need a nil check?
There was a problem hiding this comment.
There is a nil check in line 65
This is more versatile function that works for any source, not just images. It can be used together with a policy that switches between input and output source as well as for adding additional metadata for other sources in the future. Signed-off-by: Tonis Tiigi <[email protected]>
tonistiigi
force-pushed
the
sourceresolver
branch
from
9e14155
to
30c069c
Compare
AkihiroSuda
approved these changes
Feb 13, 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.
follow-up for #4209
This is more versatile function that works for any source,
not just images.
It can be used together with a policy that switches
between input and output source as well as for adding
additional metadata for other sources in the future.
@cpuguy83