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

Cleanups from PR review #1813

Merged
merged 7 commits into from
Jun 28, 2023
Merged

Cleanups from PR review #1813

merged 7 commits into from
Jun 28, 2023

Conversation

natalieparellano
Copy link
Member

@natalieparellano natalieparellano commented Jun 27, 2023 •
edited
Loading

Hopefully simplifying some things from #1787 (pointing to main for now, just to see the tests run)

@natalieparellano
Cleanups from PR review
6f50a75 
Signed-off-by: Natalie Arellano <narellano@vmware.com>
@natalieparellano natalieparellano requested review from a team as code owners June 27, 2023 16:38
@github-actions github-actions bot added this to the 0.30.0 milestone Jun 27, 2023
@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Jun 27, 2023
@natalieparellano natalieparellano marked this pull request as draft June 27, 2023 17:15
@natalieparellano natalieparellano changed the base branch from fix/flatten-tar-extras-2 to main June 27, 2023 17:15
@natalieparellano
Fix more stuff
e7c0a12 
Signed-off-by: Natalie Arellano <narellano@vmware.com>
@github-actions github-actions bot added the type/chore Issue that requests non-user facing changes. label Jun 27, 2023
@natalieparellano
Update comment
cead534 
Signed-off-by: Natalie Arellano <narellano@vmware.com>
github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 27, 2023
View reviewed changes
pkg/buildpack/buildpack.go
)

for {
header, err = tr.Next()

Check failure

Code scanning / CodeQL

Arbitrary file write during zip extraction ("zip slip")

Unsanitized archive entry, which may contain '..', is used in a [file system operation](1).
@natalieparellano
Variable rename and pend tests to see if acceptance is passing
e14d0d9 
Signed-off-by: Natalie Arellano <narellano@vmware.com>
@natalieparellano
Fix units by not writing parent headers twice
03116fe 
Signed-off-by: Natalie Arellano <narellano@vmware.com>
github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 27, 2023
View reviewed changes
pkg/buildpack/buildpack.go Fixed Show fixed Hide fixed
pkg/buildpack/buildpack.go Outdated
// write the rest of the package
var header *tar.Header
for {
header, err = tr.Next()

Check failure

Code scanning / CodeQL

Arbitrary file write during zip extraction ("zip slip")

Unsanitized archive entry, which may contain '..', is used in a [file system operation](1).
github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 27, 2023
View reviewed changes
pkg/buildpack/buildpack.go
if origVersion == "" {
// the first header only contains the id - e.g., /cnb/buildpacks/<buildpack-id>,
// read the next header to get the version
secondHeader, err := tr.Next()

Check failure

Code scanning / CodeQL

Arbitrary file write during zip extraction ("zip slip")

Unsanitized archive entry, which may contain '..', is used in a [file system operation](1).
@natalieparellano natalieparellano force-pushed the fix/flatten-tar-extras-3 branch 3 times, most recently from a2b867e to d43dfb9 Compare June 27, 2023 20:14
@natalieparellano
Try to fix Windows
84ee822 
Signed-off-by: Natalie Arellano <narellano@vmware.com>
@natalieparellano
Copy link
Member Author

Finally all the tests are passing (though Codecov & CodeQL are not happy): https://github.com/buildpacks/pack/actions/runs/5394788057/jobs/9796440626?pr=1813

I'll point this back to the feature branch for @jjbustamante to review...

@natalieparellano natalieparellano changed the base branch from main to fix/flatten-tar-extras-2 June 27, 2023 22:08
natalieparellano
natalieparellano commented Jun 27, 2023
View reviewed changes
pkg/buildpack/buildpack.go
Comment on lines -359 to -361
if !module.ContainsFlattenedModules() {
return handleSingleOrEmptyModule(dest, module)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The recursive function can handle the case where the module contains a single buildpack

@natalieparellano
Other small cleanups
d5cc7d9 
Signed-off-by: Natalie Arellano <narellano@vmware.com>
@github-actions github-actions bot removed the type/chore Issue that requests non-user facing changes. label Jun 28, 2023
jjbustamante
jjbustamante approved these changes Jun 28, 2023
View reviewed changes
pkg/buildpack/package.go
blobOpts = append(blobOpts, Flattened())
}
if desc.Info().Match(md.ModuleInfo) { // This is the order buildpack of the package
if desc.Info().Match(md.ModuleInfo) { // Current module is the order buildpack of the package
mainBP = FromBlob(&desc, b, blobOpts...)
Copy link
Member

Choose a reason for hiding this comment

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

Because we are removing method ContainsFlattenedModules(), I think we can also remove the BlobOption

@jjbustamante jjbustamante merged commit d5cc7d9 into fix/flatten-tar-extras-2 Jun 28, 2023
@jjbustamante jjbustamante deleted the fix/flatten-tar-extras-3 branch June 28, 2023 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jjbustamante jjbustamante jjbustamante approved these changes

Assignees
No one assigned
Labels
type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Milestone
0.30.0
Development

Successfully merging this pull request may close these issues.
2 participants
@natalieparellano @jjbustamante