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

bug: Update getFileFilter to include parent folders #1439

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

imnitishng
Copy link
Contributor

Summary

  • The include directive when specified with a file inside a
    folder testdir/testfile did not pass the parent folder to
    the builder image, which led to the folder being created by
    docker instead pack causing the permissions of the folder
    to be cnb.
  • This PR fixes the include logic to get parent folders and
    add them to the file filter, so they get created via pack.

Output

Before

After

Documentation

  • Should this change be documented?
    • Yes, see #___
    • No

Related

Resolves #1419

@imnitishng imnitishng requested a review from a team as a code owner May 11, 2022 20:58
@imnitishng imnitishng marked this pull request as draft May 11, 2022 20:58
@github-actions github-actions bot added this to the 0.27.0 milestone May 11, 2022
@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label May 11, 2022
@imnitishng imnitishng force-pushed the 12-05-include-directive-fix branch from 48a493d to a76e5f8 Compare May 11, 2022 20:59
@imnitishng
Copy link
Contributor Author

The PR focuses on include part of the issue for now.
Will add more unit/acceptance tests here.

@imnitishng imnitishng force-pushed the 12-05-include-directive-fix branch from a76e5f8 to e4081d0 Compare May 12, 2022 14:58
@github-actions github-actions bot added the type/chore Issue that requests non-user facing changes. label May 12, 2022
@imnitishng imnitishng force-pushed the 12-05-include-directive-fix branch 3 times, most recently from e7f0b86 to ab66f6b Compare May 13, 2022 16:13
@imnitishng imnitishng marked this pull request as ready for review May 13, 2022 16:13
@imnitishng imnitishng force-pushed the 12-05-include-directive-fix branch from ab66f6b to 55e3f0e Compare May 13, 2022 16:26
- The `include` directive when specified with a file inside a
  folder `testdir/testfile` did not pass the parent folder to
  the builder image, which led to the folder being created by
  docker instead pack causing the permissions of the folder
  to be `cnb`.
- This PR fixes the include logic to get parent folders and
  add them to the file filter, so they get created via pack.

Signed-off-by: Nitish Gupta <[email protected]>
@imnitishng imnitishng force-pushed the 12-05-include-directive-fix branch from 55e3f0e to c1af702 Compare May 13, 2022 17:21
@imnitishng
Copy link
Contributor Author

Hey @jromero,
I have tried to fix the include part of the issue based on #1419 (comment)
The idea is to add parent folder entries to the fileFilter so the folders are included in the final build.
The approach works fine for cases when we know the directory name (for ex: testdir/testfile.txt, testdir/abc/name.txt, etc) but it would not work when we do not know the folder name as in Regex (for ex: abc/*/name.txt, test/**/file.txt, this would cause abc/def/ to have root owner, since we did not add ["!abc/def/*", "abc/def/name.txt"] to the filter).
I don't see a way where we could tweak the getFileFilter to include such cases, I'd like to know if there's a way I'm missing.
Moreover if we DO want to cover these cases then I guess we will have to modify WriteDirToTar to achieve this, but I'm not sure if the added complexity would be worth.
For now I have added a check that reverts back to old logic on encountering a regex containing either of *.
The compatibility tests also fail due to the changes.

@jromero jromero modified the milestones: 0.27.0, 0.28.0 Jun 3, 2022
@jromero jromero modified the milestones: 0.28.0, 0.29.0 Sep 16, 2022
@dfreilich dfreilich modified the milestones: 0.29.0, 0.30.0 Mar 26, 2023
@natalieparellano natalieparellano modified the milestones: 0.30.0, 0.31.0 May 19, 2023
@jjbustamante jjbustamante modified the milestones: 0.32.0, 0.33.0 Oct 17, 2023
@natalieparellano natalieparellano modified the milestones: 0.33.0, 0.34.0 Dec 13, 2023
@jjbustamante jjbustamante removed this from the 0.34.0 milestone Mar 8, 2024
@jjbustamante jjbustamante added this to the 0.35.0 milestone Mar 8, 2024
@natalieparellano natalieparellano modified the milestones: 0.35.0, 0.36.0 Jul 9, 2024
@natalieparellano natalieparellano modified the milestones: 0.36.0, 0.37.0 Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/chore Issue that requests non-user facing changes. type/enhancement Issue that requests a new feature or improvement.
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

include/exclude directives can lead to directories that are not writable at build time
5 participants