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

Fix missing chown/chmod when using parents flag with ADD/COPY command #4598

Merged
merged 1 commit into from
Jan 31, 2024

Conversation

nobiit
Copy link
Contributor

@nobiit nobiit commented Jan 30, 2024

When using the parents flag, it will add CopyInfo to the end of opts

if cfg.parents {
path := strings.TrimPrefix(src, "/")
opts = append(opts, &llb.CopyInfo{
IncludePatterns: []string{path},
})
src = "/"
}

And since it's the last, it will replace any opts in front. So the chmod/chown flags in front will be disabled (like they don't exist).

func (mi *CopyInfo) SetCopyOption(mi2 *CopyInfo) {
*mi2 = *mi
}

We should take it forward. Like this

opts := append([]llb.CopyOption{&llb.CopyInfo{
Mode: mode,
CreateDestPath: true,
}}, copyOpt...)

@jedevc
Copy link
Member

jedevc commented Jan 30, 2024

Not quite sure how the change in this PR is supposed to resolve the issue?


The right way to resolve this is to create the CopyInfo struct before checking cfg.parents, and then set IncludePatterns based on that.

We can then add the CopyInfo struct to the opts list - you're right, the last one will take precedence, so we should take care to only add it once.

@nobiit
Copy link
Contributor Author

nobiit commented Jan 31, 2024

Thanks for @jedevc suggestion, I have rewritten the code the way you suggested. I think it will be clearer and easier to read

@jedevc
Copy link
Member

jedevc commented Jan 31, 2024

Thanks @nobiit! 🎉 🎉 (welcome to buildkit btw!)

@jedevc jedevc merged commit 6b2dfbc into moby:master Jan 31, 2024
63 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.

2 participants