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.

Sign up for GitHub

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

Jump to bottom

export(local): split opt #3161

Merged
merged 1 commit into from
Jun 30, 2023
Merged

export(local): split opt #3161

merged 1 commit into from
Jun 30, 2023

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Oct 11, 2022 •
edited

adds split local exporter option which can be used to split result in subfolders when multiple references are exported (default true).

Signed-off-by: CrazyMax crazy-max@users.noreply.github.com

@crazy-max crazy-max force-pushed the local-exporter-wrap branch 2 times, most recently from c59a7a1 to 8e1272d Compare October 14, 2022 10:02
tonistiigi
tonistiigi reviewed Oct 17, 2022
View reviewed changes
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

I'm not sure if wrap is the best term for this. cc @jedevc for native speakers.

Should the build error if a file with the same name exists on multiple platforms?

@jedevc
Copy link
Member

jedevc commented Oct 18, 2022

wrap doesn't seem right, agreed.

I think merge would work (which we already use in the docs), though I'm not sure if we maybe want to reserve that name for doing something different?

Alternatively, we could have an option split (defaulting to true), and then allow setting split=false to get all the results together,

@crazy-max
Copy link
Member Author

Alternatively, we could have an option split (defaulting to true), and then allow setting split=false to get all the results together,

split sounds good.

@crazy-max crazy-max changed the title export(local): wrap opt export(local): split opt Oct 28, 2022
@tonistiigi tonistiigi added this to the v0.12.0 milestone May 4, 2023
@crazy-max crazy-max marked this pull request as ready for review May 23, 2023 09:23
jedevc
jedevc reviewed May 30, 2023
View reviewed changes
exporter/local/export.go
Copy link
Member

Choose a reason for hiding this comment

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

Could we also add this functionality to the tar exporter?

Ideally we should keep the options for these as close as possible. I think we should be able to put this opt into CreateFSOPts now that #3289 is merged.

Copy link
Member Author

Choose a reason for hiding this comment

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

now that #3289 is merged.

Ah there's quite a lot of changes indeed, let me address that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could we also add this functionality to the tar exporter?

Don't think wrapping output for tar exporter would make sense 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Why doesn't this behavior make sense for tar ?

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do tar in follow-up if ok with you. I have some issues with MergeFS when platform-split is disabled. Seems we need Stat along FS. Or a fsutil.MergeDirFS method like the SubDirFS one could be better.

exporter/local/export.go Outdated Show resolved Hide resolved
exporter/local/export.go Outdated Show resolved Hide resolved
frontend/dockerfile/dockerfile_test.go Outdated Show resolved Hide resolved
@crazy-max crazy-max force-pushed the local-exporter-wrap branch 3 times, most recently from 0a4b7f4 to 1662821 Compare May 30, 2023 11:08
tonistiigi
tonistiigi reviewed Jun 28, 2023
View reviewed changes
README.md Outdated Show resolved Hide resolved
exporter/local/export.go
Copy link
Member

Choose a reason for hiding this comment

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

Why doesn't this behavior make sense for tar ?

exporter/local/export.go Outdated Show resolved Hide resolved
exporter/local/export.go Show resolved Hide resolved
exporter/local/export.go Outdated Show resolved Hide resolved
exporter/local/fs.go Outdated Show resolved Hide resolved
@crazy-max crazy-max force-pushed the local-exporter-wrap branch 2 times, most recently from 7b8b08a to 37187c2 Compare June 29, 2023 17:32
@crazy-max
export(local): split opt
e6818cf 
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@tonistiigi tonistiigi closed this Jun 29, 2023
@tonistiigi tonistiigi reopened this Jun 29, 2023
tonistiigi
tonistiigi approved these changes Jun 30, 2023
View reviewed changes
exporter/local/export.go
visitedMu.Lock()
defer visitedMu.Unlock()
if vp, ok := visitedPath[p]; ok {
return errors.Errorf("cannot overwrite %s from %s with %s when split option is disabled", p, vp, k)
Copy link
Member

Choose a reason for hiding this comment

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

This error message could be more understandable to user. Eg. "file x exists in both platform1 and platform2 and would be lost by merging the platforms".

@tonistiigi tonistiigi merged commit 5b9a9ce into moby:master Jun 30, 2023
62 of 68 checks passed
@crazy-max crazy-max deleted the local-exporter-wrap branch June 30, 2023 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jedevc jedevc jedevc left review comments

@tonistiigi tonistiigi tonistiigi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.12.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@crazy-max @jedevc @tonistiigi