8000 refactor: lint by AustinAbro321 · Pull Request #3776 · zarf-dev/zarf · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

refactor: lint #3776

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

Merged
merged 21 commits into from
May 12, 2025
Merged

refactor: lint #3776

merged 21 commits into from
May 12, 2025

Conversation

AustinAbro321
Copy link
Contributor
@AustinAbro321 AustinAbro321 commented May 8, 2025

Description

Lint was using src/pkg/packager which will be replaced by packager2.

This comes with behavior changes in Lint that must be made to delete src/pkg/packager without greatly increasing complexity or duplicating lots of code.

  • Lint will no longer print package schema errors and warnings at the same time. Instead, if there are schema errors the command will fail earlier and will not also lint. IMO this is a sensible change. When lint was originally created zarf package create did not have schema checks so packages could have invalid schemas as still be used. Now that a package with an invalid schema will always fail create, earlier validation makes sense.
  • Lint now runs on the composed package, rather than the relevant components. Therefore, when a warning is given the path to the warning will be on the composed package rather than the original package. While I consider this to be a slightly worse UX, since every warning comes with a specific item it shouldn't be hard to determine the source of the error.
    For example, before this PR a warning looks like this:
    image
    After this PR they look like this:
    image
  • Lint now requires that package templates be set rather than warning when they are not set. IMO this is a better design and consistent with create

Checklist before merging

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321 AustinAbro321 requested review from a team as code owners May 8, 2025 12:37
Copy link
netlify bot commented May 8, 2025

Deploy Preview for zarf-docs canceled.

Name Link
🔨 Latest commit 5040244
🔍 Latest deploy log https://app.netlify.com/sites/zarf-docs/deploys/6822328c4acb910008c9e471

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Copy link
codecov bot commented May 8, 2025

Codecov Report

Attention: Patch coverage is 38.88889% with 44 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/cmd/table.go 0.00% 15 Missing ⚠️
src/internal/packager2/lint.go 48.14% 9 Missing and 5 partials ⚠️
src/pkg/lint/schema.go 52.94% 5 Missing and 3 partials ⚠️
src/cmd/dev.go 0.00% 5 Missing ⚠️
src/pkg/lint/findings.go 75.00% 2 Missing ⚠️
Files with missing lines Coverage Δ
src/internal/packager2/layout/create.go 42.33% <ø> (+0.04%) ⬆️
src/pkg/packager/creator/utils.go 14.81% <ø> (+0.26%) ⬆️
src/pkg/packager/pull.go 0.00% <ø> (ø)
src/pkg/lint/findings.go 46.15% <75.00%> (-26.07%) ⬇️
src/cmd/dev.go 47.17% <0.00%> (-0.25%) ⬇️
src/pkg/lint/schema.go 55.26% <52.94%> (+1.29%) ⬆️
src/internal/packager2/lint.go 48.14% <48.14%> (ø)
src/cmd/table.go 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
@AustinAbro321 AustinAbro321 marked this pull request as draft May 8, 2025 15:50
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleting this as I introduced this script alongside the zarf dev lint command, and have not used the script since. Additionally, because zarf dev lint now forces uses to define package templates (this behavior is consistent with zarf package create) the script will break on packages with templates

@AustinAbro321 AustinAbro321 marked this pull request as ready for review May 12, 2025 12:16
)

// Lint lints the given Zarf package
func Lint(ctx context.Context, packagePath, flavor string, setVariables map[string]string) error {
Copy link
Contributor
@mkcp mkcp May 12, 2025

Choose a reason for hiding this comment

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

blocking: packagePath appears to be a required param here, i'd recommend checking if the path is empty string (and, even better, checking if isn't a valid path) and providing a semantic error to the user if so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an empty check to packagePath. IMO it's better to leave the valid path checks to the os package. It will be caught right away.

return err
}

// [DEPRECATION] Set the Package Variable syntax as well for backward compatibility
Copy link
Contributor

Choose a reason for hiding this comment

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

Good comment 👍

// LintError represents an error containing lint findings.
//
//nolint:revive // ignore name
type LintError struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Error types ++

Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Copy link
Contributor
@mkcp mkcp left a comment

Choose a reason for hiding this comment

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

LGTM

@AustinAbro321 AustinAbro321 added this pull request to the merge queue May 12, 2025
Merged via the queue into main with commit 2f52235 May 12, 2025
26 checks passed
@AustinAbro321 AustinAbro321 deleted the refactor-lint branch May 12, 2025 19:33
nevinaragam pushed a commit to nevinaragam/zarf that referenced this pull request May 20, 2025
Signed-off-by: Austin Abro <AustinAbro321@gmail.com>
Signed-off-by: NevinAragam <nevin.aragam@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0