-
Notifications
You must be signed in to change notification settings - Fork 43
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
Conversation
c1ca00e
to
cd3cb2e
Compare
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.
Nice work! A couple of nits but nothing blocking from me.
// 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 | ||
} |
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.
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 :(
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 agree 😔, hopefully it's rarely needed.
if stage.BaseName == "scratch" { | ||
return "", nil | ||
} |
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.
Confirmed that building FROM scratch
results in a manifest with User: ""
.
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.
Nice job 👍 👍
Co-authored-by: Cian Johnston <[email protected]>
Co-authored-by: Cian Johnston <[email protected]> (cherry picked from commit df8ea67)
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.