fix: pulling not respecting "uncompressed" setting in metadata by a1994sc · Pull Request #3472 · zarf-dev/zarf

a1994sc · 2025-02-05T15:56:14Z

Description

Address zarf package pull not respecting .metadata.uncompressed package setting

Related Issue

Fixes #3464

Checklist before merging

Test, docs, adr added or updated as needed
Contributor Guide Steps followed

Signed-off-by: Allen Conlon <software@conlon.dev>

netlify · 2025-02-05T15:56:43Z

✅ Deploy Preview for zarf-docs canceled.

Name	Link
🔨 Latest commit	`ee055e2`
🔍 Latest deploy log	https://app.netlify.com/sites/zarf-docs/deploys/67ad162877a0060008a46102

src/internal/packager2/pull.go

Signed-off-by: Allen Conlon <software@conlon.dev>

src/internal/packager2/pull.go

Signed-off-by: Allen Conlon <software@conlon.dev>

codecov · 2025-02-10T20:55:17Z

Codecov Report

Attention: Patch coverage is 34.37500% with 42 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/internal/packager2/pull.go	35.48%	34 Missing and 6 partials ⚠️
src/internal/packager2/load.go	0.00%	2 Missing ⚠️

Files with missing lines	Coverage Δ
src/internal/packager2/load.go	`57.14% <0.00%> (ø)`
src/internal/packager2/pull.go	`33.47% <35.48%> (+4.83%)`	⬆️

Signed-off-by: Allen Conlon <software@conlon.dev>

AustinAbro321

LGTM, nice work. Going to ask another member of my team to give it a look before merge

a1994sc · 2025-02-11T14:56:13Z

I will see if I can track down that windows test failing....

Going to leave these to find easier in the future:

test-unit

=== NAME  TestPull
    pull_test.go:43: 
        	Error Trace:	D:/a/zarf/zarf/src/internal/packager2/pull_test.go:43
        	Error:      	Received unexpected error:
        	            	rename C:\Users\RUNNER~1\AppData\Local\Temp\zarf-1793982925\data C:\Users\RUNNER~1\AppData\Local\Temp\zarf-1793982925\data.tar.zst: The process cannot access the file because it is being used by another process.
        	Test:       	TestPull
--- FAIL: TestPull (0.04s)
=== CONT  TestIdentifySource/oci
=== CONT  TestIdentifySource/local_tar
=== CONT  TestLoadPackage/split
=== NAME  TestPullUncompressed
    pull_test.go:74: 
        	Error Trace:	D:/a/zarf/zarf/src/internal/packager2/pull_test.go:74
        	Error:      	Received unexpected error:
        	            	rename C:\Users\RUNNER~1\AppData\Local\Temp\zarf-2664840283\data C:\Users\RUNNER~1\AppData\Local\Temp\zarf-2664840283\data.tar: The process cannot access the file because it is being used by another process.
        	Test:       	TestPullUncompressed

test-e2e-without-cluster

2025-02-11 14:52:30 ERR rename C:\Users\RUNNER~1\AppData\Local\Temp\zarf-284784713\zarf-3669846076\data C:\Users\RUNNER~1\AppData\Local\Temp\zarf-284784713\zarf-3669846076\data.tar.zst: The process cannot access the file because it is being used by another process.
=== NAME  TestUseCLI/zarf_package_pull_https
    00_use_cli_test.go:70: 
        	Error Trace:	D:/a/zarf/zarf/src/test/e2e/00_use_cli_test.go:70
        	Error:      	Received unexpected error:
        	            	exit status 1
        	Test:       	TestUseCLI/zarf_package_pull_https

AustinAbro321 · 2025-02-11T15:36:38Z

src/internal/packager2/pull.go

 	}
-	return nil
+
+	mtype, err := mimetype.DetectFile(tarPath)


Windows doesn't allow renaming of an already opened file. Rather than defer f.close you will have to run f.close anywhere after the io.Copy

gotcha... I will have an update to the function before this afternoon for the team to review

I moved the f.Close() to after

_, err = io.Copy(f, resp.Body)

and added f.Close() on error returns. Let me know if that helps

Ah yes, good catch to make sure the files close in the error case. Still, I feel it would be simpler if we moved the code from the os.Create call to the io.Copy call into it's own function. This way we can use defer and avoid the several f.close calls in each error case.

Gotcha, I will see if I can encapsulate that logic

Moved that logic to its own function, I believe that should address the windows issues.... fingers crossed

Signed-off-by: Allen Conlon <software@conlon.dev>

AustinAbro321

LGTM, nice work

a1994sc · 2025-02-12T14:58:30Z

Thank you!

src/internal/packager2/pull.go

…mpressed

Signed-off-by: Allen Conlon <software@conlon.dev>

…dev#3472) Signed-off-by: Allen Conlon <software@conlon.dev> Signed-off-by: NevinAragam <nevin.aragam@gmail.com>

fix: pulling not respecting setting

d21a84c

Signed-off-by: Allen Conlon <software@conlon.dev>

a1994sc marked this pull request as ready for review February 5, 2025 15:56

a1994sc requested review from a team as code owners February 5, 2025 15:56

AustinAbro321 requested changes Feb 6, 2025

View reviewed changes

src/internal/packager2/pull.go Outdated Show resolved Hide resolved

feat: updated logic for pulling uncompressed

ee1caf0

Signed-off-by: Allen Conlon <software@conlon.dev>

a1994sc force-pushed the hotfix/pull-uncompressed branch from 7d41fa3 to ee1caf0 Compare February 7, 2025 20:36

a1994sc commented Feb 7, 2025

View reviewed changes

src/internal/packager2/pull.go Show resolved Hide resolved

feat: updated pullHTTP to do type mime type checking

12d7dff

Signed-off-by: Allen Conlon <software@conlon.dev>

a1994sc force-pushed the hotfix/pull-uncompressed branch from aebc000 to 12d7dff Compare February 8, 2025 00:42

a1994sc changed the title ~~fix: pulling not respecting setting~~ fix: pulling not respecting "uncompressed" setting in metadata Feb 10, 2025

Merge branch 'main' into hotfix/pull-uncompressed

c39afc8

a1994sc requested a review from AustinAbro321 February 10, 2025 18:30

chore(go): tidy

b8d559a

Signed-off-by: Allen Conlon <software@conlon.dev>

a1994sc force-pushed the hotfix/pull-uncompressed branch from e3a273a to b8d559a Compare February 10, 2025 22:06

feat: updated logic

f1fd441

Signed-off-by: Allen Conlon <software@conlon.dev>

AustinAbro321 previously approved these changes Feb 11, 2025

View reviewed changes

AustinAbro321 reviewed Feb 11, 2025

View reviewed changes

fix(e2e): copy logic for windows environment

f7378ba

Signed-off-by: Allen Conlon <software@conlon.dev>

a1994sc dismissed AustinAbro321’s stale review via f7378ba February 11, 2025 16:27

a1994sc added 2 commits February 11, 2025 11:29

fix(e2e): added closing logic when sending error

5ae662c

Signed-off-by: Allen Conlon <software@conlon.dev>

feat(go): encapsulate logic for file into its own function

13c632c

Signed-off-by: Allen Conlon <software@conlon.dev>

a1994sc force-pushed the hotfix/pull-uncompressed branch from e98d876 to 13c632c Compare February 11, 2025 21:08

AustinAbro321 previously approved these changes Feb 12, 2025

View reviewed changes

brandtkeller reviewed Feb 12, 2025

View reviewed changes

src/internal/packager2/pull.go Outdated Show resolved Hide resolved

Merge remote-tracking branch 'origin-zarf/main' into hotfix/pull-unco…

9e4db26

…mpressed

chore(go): removed unused variable

ee055e2

Signed-off-by: Allen Conlon <software@conlon.dev>

a1994sc dismissed AustinAbro321’s stale review via ee055e2 February 12, 2025 21:44

brandtkeller approved these changes Feb 13, 2025

View reviewed changes

AustinAbro321 approved these changes Feb 13, 2025

View reviewed changes

AustinAbro321 added this pull request to the merge queue Feb 13, 2025

Merged via the queue into zarf-dev:main with commit 8a09e8d Feb 13, 2025
26 checks passed

nevinaragam pushed a commit to nevinaragam/zarf that referenced this pull request May 20, 2025

fix: pulling not respecting "uncompressed" setting in metadata (zarf-…

fd5e925

…dev#3472) Signed-off-by: Allen Conlon <software@conlon.dev> Signed-off-by: NevinAragam <nevin.aragam@gmail.com>

Conversation

a1994sc commented Feb 5, 2025

Description

Related Issue

Checklist before merging

Uh oh!

netlify bot commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for zarf-docs canceled.

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AustinAbro321 left a comment

Choose a reason for hiding this comment

Uh oh!

a1994sc commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AustinAbro321 Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

a1994sc Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

a1994sc Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

AustinAbro321 Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

a1994sc Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

a1994sc Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AustinAbro321 left a comment

Choose a reason for hiding this comment

Uh oh!

a1994sc commented Feb 12, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

netlify bot commented Feb 5, 2025 •

edited

Loading

codecov bot commented Feb 10, 2025 •

edited

Loading

a1994sc commented Feb 11, 2025 •

edited

Loading

a1994sc Feb 11, 2025 •

edited

Loading