Skip to content
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

add missing fallback from new frontend to daemon without sourceresolver #4647

Merged
merged 2 commits into from Feb 14, 2024

Conversation

tonistiigi
Copy link
Member

This fallback case was accidentally left out of https://github.com/moby/buildkit/pull/4563/files

It can appear for example if new dockerfile frontend resolves SBOM generator image.

@@ -484,7 +484,31 @@ func (c *grpcClient) Solve(ctx context.Context, creq client.SolveRequest) (res *

func (c *grpcClient) ResolveSourceMetadata(ctx context.Context, op *opspb.SourceOp, opt sourceresolver.Opt) (*sourceresolver.MetaResponse, error) {
if c.caps.Supports(pb.CapSourceMetaResolver) != nil {
return nil, errors.Errorf("fallback not implemented")
var ref string
if strings.HasPrefix(op.Identifier, "docker-image://") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strings.CutPrefix can replace HasPrefix and TrimPrefix.

if strings.HasPrefix(retRef, "docker-image://") {
op.Identifier = "docker-image://" + retRef
} else if strings.HasPrefix(retRef, "oci-layout://") {
op.Identifier = "oci-layout://" + retRef
Copy link
Member

@jedevc jedevc Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If HasPrefix, prepend it again? I assume the line above should be op.Identifier.

Copy link
Member

@jedevc jedevc Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is still wrong. ref has had the prefix stripped already, so the condition never triggers.

This should be:

if strings.HasPrefix(op.Identifier, "docker-image://") {
	op.Identifier = "docker-image://" + retRef
}

That was we actually are checking the prefix, and making sure the output has the same prefix.

@tonistiigi tonistiigi force-pushed the sourceresolver-fallback branch 2 times, most recently from f18756f to 4fe9630 Compare February 14, 2024 16:27
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@tonistiigi
Copy link
Member Author

Self-merge to unblock CI

@tonistiigi tonistiigi merged commit f1bd3af into moby:master Feb 14, 2024
68 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants