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: normal creator #3114

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

refactor: normal creator #3114

wants to merge 1 commit into from

Conversation

phillebaba
Copy link
Member

@phillebaba phillebaba commented Oct 17, 2024 •
edited
Loading

Description

This change refactors the package creation.

Related Issue

Relates to #2969

Checklist before merging

@netlify Netlify
Copy link

netlify bot commented Oct 17, 2024 •
edited
Loading

✅ Deploy Preview for zarf-docs canceled.

Name Link
🔨 Latest commit dcd8b19
🔍 Latest deploy log https://app.netlify.com/sites/zarf-docs/deploys/6745914cbd56b600084f5998

@codecov Codecov
Copy link

codecov bot commented Oct 17, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 18.94996% with 988 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/internal/packager2/layout/create.go 3.91% 436 Missing and 6 partials ⚠️
src/internal/packager2/layout/sbom.go 9.50% 213 Missing and 6 partials ⚠️
src/internal/packager2/actions/actions.go 0.00% 208 Missing ⚠️
src/internal/packager2/create.go 0.00% 40 Missing ⚠️
src/internal/packager2/filters/deploy.go 76.85% 23 Missing and 5 partials ⚠️
src/internal/packager2/layout/package.go 0.00% 26 Missing ⚠️
src/cmd/package.go 0.00% 13 Missing ⚠️
src/internal/packager2/filters/diff.go 84.61% 4 Missing and 2 partials ⚠️
src/internal/packager2/layout/oci.go 0.00% 6 Missing ⚠️
Files with missing lines Coverage Δ
src/internal/packager2/filters/empty.go 100.00% <100.00%> (ø)
src/internal/packager2/filters/os.go 100.00% <100.00%> (ø)
src/internal/packager2/filters/select.go 100.00% <100.00%> (ø)
src/internal/packager2/filters/strat.go 100.00% <100.00%> (ø)
src/internal/packager2/filters/utils.go 100.00% <100.00%> (ø)
src/internal/packager2/layout/layout.go 70.00% <100.00%> (-2.73%) ⬇️
src/internal/packager2/filters/diff.go 84.61% <84.61%> (ø)
src/internal/packager2/layout/oci.go 22.10% <0.00%> (ø)
src/cmd/package.go 0.00% <0.00%> (ø)
src/internal/packager2/layout/package.go 40.54% <0.00%> (-4.53%) ⬇️
... and 5 more

🚨 Try these New Features:

@phillebaba phillebaba force-pushed the refactor/creator-normal branch 7 times, most recently from 47c5dd1 to 5b54240 Compare October 25, 2024 09:00
@phillebaba phillebaba force-pushed the refactor/creator-normal branch 9 times, most recently from cbd05e3 to ca69d35 Compare November 2, 2024 15:49
@phillebaba phillebaba force-pushed the refactor/creator-normal branch 6 times, most recently from 3c36c9d to 4a829c6 Compare November 5, 2024 21:57
@phillebaba phillebaba marked this pull request as ready for review November 5, 2024 21:58
@phillebaba phillebaba requested review from a team as code owners November 5, 2024 21:58
phillebaba
phillebaba commented Nov 5, 2024
View reviewed changes
src/internal/packager2/layout/layout.go Outdated
for idx, component := range pkg.Components {
pkg.Components[idx], _ = deprecated.MigrateComponent(pkg.Build, component)
}
// TODO (phillebaba): Figure out when migrations should actually be run.
Copy link
Member Author

Choose a reason for hiding this comment

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

Address this.

AustinAbro321
AustinAbro321 reviewed Nov 6, 2024
View reviewed changes
Copy link
Contributor

@AustinAbro321 AustinAbro321 left a comment

Choose a reason for hiding this comment

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

Great work, love that we are no longer changing directory. Left a few comments

src/cmd/package.go
if err != nil {
return err
}
defer pkgClient.ClearTempPaths()
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we should continue to print the lint table. The error message currently is
linting error found 1 instance(s), which doesn't provide enough context to easily figure out what went wrong

Copy link
Member Author

Choose a reason for hiding this comment

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

We print linting errors in a lot of places, and this creates a lot of noise and complexity. If you look at a lot of other tools they will not lint during build, instead it is a separate command. We have a separate command today which users should use.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I added the printing back, but we should evaluate this in the future.

src/internal/packager2/actions/actions.go Outdated Show resolved Hide resolved
src/internal/packager2/layout/create.go
if err != nil {
return err
}
defer os.RemoveAll(tmpBuildPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be tmpDir

Copy link
Member Author

Choose a reason for hiding this comment

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

tmpDir is already used in this function.

src/internal/packager2/layout/create_test.go Outdated Show resolved Hide resolved
src/internal/packager2/layout/sbom_test.go Outdated Show resolved Hide resolved
@phillebaba phillebaba force-pushed the refactor/creator-normal branch 10 times, most recently from 9141561 to 60a658b Compare November 25, 2024 13:45
@phillebaba
refactor: normal creator
dcd8b19 
Signed-off-by: Philip Laine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AustinAbro321 AustinAbro321 AustinAbro321 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
ready-for-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@phillebaba @AustinAbro321