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

refactor(pkg/archive): factor out createImpliedDirectories helper #44196

Merged
merged 2 commits into from
Sep 27, 2022
Merged

refactor(pkg/archive): factor out createImpliedDirectories helper #44196

merged 2 commits into from
Sep 27, 2022

Conversation

neersighted
Copy link
Member

@neersighted neersighted commented Sep 26, 2022 •
edited
Loading

- What I did

  • Factor out common 'implied' directory creation code in pkg/archive into a helper.
  • Update the non-diff/overlay codepath to use the same permissions mask as the (more common) overlay codepath.
  • Document why the constant 0755 was chosen, and why image authors cannot currently rely on this value given it is not standardized/is fully implementation-defined.
  • Add tests to ensure the constant is used and directories are created with the expected permissions.

- How to verify it
CI

- Description for the changelog
Update the default permissions for directories implied but not explicitly defined by a layer to 0755 across all graphdrivers.

- A picture of a cute animal (not mandatory but encouraged)
image

@neersighted neersighted added this to the v-next milestone Sep 26, 2022
@neersighted
Copy link
Member Author

For additional context regarding why this value is presently unreliable/implementation defined, see #44106 (comment).

corhere
corhere reviewed Sep 26, 2022
View reviewed changes
pkg/archive/archive.go Show resolved Hide resolved
pkg/archive/archive.go Outdated Show resolved Hide resolved
pkg/archive/archive.go Outdated Show resolved Hide resolved
pkg/archive/archive_test.go Outdated Show resolved Hide resolved
pkg/archive/archive_test.go Outdated Show resolved Hide resolved
pkg/archive/archive_test.go Outdated Show resolved Hide resolved
pkg/archive/archive_test.go Outdated Show resolved Hide resolved
@neersighted @vasiliy-ul
refactor(pkg/archive): factor out createImpliedDirectories helper
4831ff9 
This code was duplicated in two places -- factor it out, add
documentation, and move magic numbers into a constant.

Additionally, use the same permissions (0755) in both code paths, and
ensure that the ID map is used in both code paths.

Co-authored-by: Vasiliy Ulyanov <vulyanov@suse.de>
Signed-off-by: Bjorn Neergaard <bneergaard@mirantis.com>
Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>
@neersighted @corhere
test(pkg/archive): add TestImpliedDirectoryPermissions
5dff494 
Co-authored-by: Cory Snider <csnider@mirantis.com>
Signed-off-by: Bjorn Neergaard <bneergaard@mirantis.com>
corhere
corhere approved these changes Sep 26, 2022
View reviewed changes
Copy link
Contributor

@corhere corhere left a comment

Choose a reason for hiding this comment

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

:shipit:

@thaJeztah thaJeztah mentioned this pull request Sep 27, 2022
Merged
thaJeztah
thaJeztah reviewed Sep 27, 2022
View reviewed changes
pkg/archive/archive.go Show resolved Hide resolved
pkg/archive/archive.go Show resolved Hide resolved
This was referenced Sep 27, 2022
Merged
Closed
thaJeztah
thaJeztah approved these changes Sep 27, 2022
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@thaJeztah
Copy link
Member

arf... I edited the top description, but that triggered another run of Jenkins, well 🤷‍♂️

@neersighted
Copy link
Member Author

arf... I edited the top description, but that triggered another run of Jenkins, well 🤷‍♂️

For what it's worth, I already included the issue linking in the body, but at the bottom. I'll adopt the bulleted-on-top style in the future 😅

@thaJeztah
Copy link
Member

For what it's worth, I already included the issue linking in the body, but at the bottom.

Oh! I completely overlooked.

I nowadays also use the bullet-list formatting (where suitable), which renders the links with the linked description; not always useful, but sometimes saves me from having to "mouse-hover" (or "click") to see the linked issue/

@thaJeztah thaJeztah merged commit a4f3c08 into moby:master Sep 27, 2022
@neersighted neersighted deleted the createImpliedDirectories branch September 27, 2022 17:46
@neersighted
Copy link
Member Author

@thaJeztah I've gone ahead and applied the cherry-pick label since backporting this was mentioned. Feel free to remove it if you conclude that it's not worth backporting (likewise, I am happy to backport on request, but don't have any special interest personally).

@thaJeztah
Copy link
Member

@neersighted ah, thanks; thought it was on there already; yes, if you have time to prepare a cherry pick, that'd be good 👍

@neersighted neersighted mentioned this pull request Sep 27, 2022
Merged
@neersighted
Copy link
Member Author

@neersighted ah, thanks; thought it was on there already; yes, if you have time to prepare a cherry pick, that'd be good 👍

#44207 is up

@thaJeztah thaJeztah mentioned this pull request Feb 9, 2023
Open
@thaJeztah thaJeztah mentioned this pull request Jun 7, 2023
Closed
myback added a commit to myback/docker_pull that referenced this pull request Nov 6, 2023
@myback
tar header changed after moby [refactoring](moby/moby#44196)
27bea1f
myback added a commit to myback/docker_pull that referenced this pull request Nov 6, 2023
@myback
tar header changed after moby refactoring moby/moby#44196
0157032
myback added a commit to myback/docker_pull that referenced this pull request Nov 6, 2023
@myback
tar header changed after moby refactoring moby/moby#44196
582bc84
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corhere corhere corhere approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees
No one assigned
Labels
area/images impact/changelog kind/refactor PR's that refactor, or clean-up code process/cherry-picked status/4-merge
Projects
None yet
Milestone
24.0.0
Development

Successfully merging this pull request may close these issues.

Wrong directory permissions with btrfs or devicemapper storage drivers
3 participants
@neersighted @thaJeztah @corhere