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: create tree of dir content types #431

Merged
merged 3 commits into from Dec 28, 2021
Merged

fix: create tree of dir content types #431

merged 3 commits into from Dec 28, 2021

Conversation

caarlos0
Copy link
Member

@caarlos0 caarlos0 commented Dec 28, 2021

Signed-off-by: Carlos A Becker <caarlos0@gmail.com>
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 28, 2021
@caarlos0 caarlos0 requested a review from a team December 28, 2021 14:03
@caarlos0 caarlos0 added the bug Something isn't working label Dec 28, 2021
Signed-off-by: Carlos A Becker <caarlos0@gmail.com>
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 28, 2021
@vercel vercel bot temporarily deployed to Preview December 28, 2021 14:04 Inactive
@caarlos0 caarlos0 changed the title fix(apk): create tree of dir types fix: create tree of dir content types Dec 28, 2021
Signed-off-by: Carlos A Becker <caarlos0@gmail.com>
@vercel vercel bot temporarily deployed to Preview December 28, 2021 14:11 Inactive
@erikgeiser
Copy link
Member

This solution assumes that the contents are sorted because by creating the parent tree of dir you rely on the fact that none of the following dir entries (that possibly have different permissions, etc.) are already including in the parent tree of the earlier entry.

Now we do sort the contents, but in a completely different package. Keep in mind that just assuming order in the packagers might be a huge foot gun for those that use nfpm as a library.

@codecov
Copy link

codecov bot commented Dec 28, 2021

Codecov Report

Merging #431 (303c70b) into main (ebe9ddf) will decrease coverage by 0.21%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #431      +/-   ##
==========================================
- Coverage   66.73%   66.52%   -0.22%     
==========================================
  Files          15       15              
  Lines        1876     1882       +6     
==========================================
  Hits         1252     1252              
- Misses        487      491       +4     
- Partials      137      139       +2     
Impacted Files Coverage Δ
apk/apk.go 72.07% <0.00%> (-0.63%) ⬇️
deb/deb.go 70.16% <0.00%> (-0.44%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebe9ddf...303c70b. Read the comment docs.

@caarlos0
Copy link
Member Author

indeed, should we maybe sort the files in a different place then (in each impl)?

or add a godoc saying "if you use this directly you'll need to sort the files first"?

thoughts?

@erikgeiser
Copy link
Member

Well we already create the tree of the dir entries already, just in the next loop.

I think the best solution would require some refactoring: We should build the actual tar contents first without writing them. Then each content entry can decide if it should overwrite existing entries or not. Like createTree would do nothing if an directory already exists and a dir entry would override an existing dir that may have been created by createTree before. Then we write it all afterwards. In this case order would be just cosmetics.

@caarlos0
Copy link
Member Author

that seems like a rather big refactor... I think I'll release this as-is to fix the bug, and create an issue so we can investigate it in the future

@caarlos0 caarlos0 merged commit d16adac into main Dec 28, 2021
@caarlos0 caarlos0 deleted the tree-dir-apk branch December 28, 2021 17:24
@github-actions github-actions bot added this to the 2.11.0 milestone Dec 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants