Skip to content

fix(devcontainer): parse user in multi-stage builds #343

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

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

mafredri
Copy link
Member

@mafredri mafredri commented Sep 11, 2024

Fixes #231


There's some more assumption and parsing fixing I want to do in devcontainer package, but I'll make other PRs for those.

@mafredri mafredri self-assigned this Sep 11, 2024
@mafredri mafredri requested review from johnstcn and mtojek September 11, 2024 17:21
@mafredri mafredri force-pushed the mafredri/fix-devcontainer-multistage-parsing branch from c1ca00e to cd3cb2e Compare September 11, 2024 17:24
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

Nice work! A couple of nits but nothing blocking from me.

Comment on lines +373 to 384
// If we can't find a user command, try to find the user from
// the image.
ref, err := name.ParseReference(strings.TrimSpace(stage.BaseName))
if err != nil {
return "", fmt.Errorf("parse image ref %q: %w", stage.BaseName, err)
}
user, err := UserFromImage(ref)
if err != nil {
return "", fmt.Errorf("user from image %s: %w", ref.Name(), err)
}
return user, nil
}
Copy link
Member

Choose a reason for hiding this comment

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

It's unfortunate to have to reach out to the remote repo to get the image manifest, but it looks like this is our only option since there's no local Docker daemon to ask :(

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree 😔, hopefully it's rarely needed.

Comment on lines +360 to +362
if stage.BaseName == "scratch" {
return "", nil
}
Copy link
Member

Choose a reason for hiding this comment

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

Confirmed that building FROM scratch results in a manifest with User: "".

Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

Nice job 👍 👍

@mafredri mafredri merged commit df8ea67 into main Sep 12, 2024
4 checks passed
@mafredri mafredri deleted the mafredri/fix-devcontainer-multistage-parsing branch September 12, 2024 10:25
johnstcn pushed a commit that referenced this pull request Sep 26, 2024
Co-authored-by: Cian Johnston <[email protected]>
(cherry picked from commit df8ea67)
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.

devcontainer: support multi-stage build with dangling build stage
3 participants