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

Initial implementation of lint subcommand #2

Merged
merged 3 commits into from
Dec 15, 2022

Conversation

Dentrax
Copy link
Member

@Dentrax Dentrax commented Dec 15, 2022

This PR initially implements brand-new lint subcommand as we discussed: wolfi-dev/os#86

  • Implement bare-minimum lint subcommand
  • Add 3 rules: no-makefile-entry-for-package, valid-copyright-header and valid-pipeline-fetch-uri (for test purposes)
  • Ran gofumpt -w -extra . across project (some intentional changes has made)
  • Move some Melange-related funcs to melange.go for code deduplication
  • No tests written yet, we thought we can do once we get agreement on the overall architecture
  • Bumped the Melange to latest version, so it brings some new dependencies.

Future ideas:

  • Create a new /rules folder to separate rules (as godolint did)
  • Give ID for each rule? (i.e., WL0001)
  • Better output format (table output, nicer log printing)
  • Verbose mode to print some additional description and stuff
  • Rich options (fail-fast, exclude/include rules by IDs, override exit code by severity, etc.)

Known Bug?:

  • We couldn't add strict field checking while unmarshaling given YAML since secfixes didn't introduce in build.Configuration yet on main branch but generally used in the package files.

Example Output:

$ go run . lint os -v
found 198 packages
2022/12/15 12:37:23 Package: wait-for-it: 1 error occurred:
	* [no-makefile-entry-for-package]: package wait-for-it is not exist in the Makefile (ERROR) - (every package should have a corresponding entry in Makefile)

(Exit code is 0, should we return 1? (if severity >= ERROR))

/cc @rawlingsj @developer-guy

Signed-off-by: Furkan furkan.turkal@trendyol.com
Co-authored-by: Batuhanbatuhan.apaydin@trendyol.com

Related to wolfi-dev/os#86

Signed-off-by: Furkan <furkan.turkal@trendyol.com>
Co-authored-by: Batuhan<batuhan.apaydin@trendyol.com>
Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>
@Dentrax Dentrax changed the title Initial implementation of lint command Initial implementation of lint subcommand Dec 15, 2022
@rawlingsj
Copy link
Member

this looks great!

I think we can merge on the agreement of a followup PR to add tests if that's ok with you?

(Exit code is 0, should we return 1? (if severity >= ERROR))

Yeah let's return 1 so that we can easily fail CI?

@rawlingsj
Copy link
Member

Or if you prefer to add a test to this PR that's probably better and good practice. To answer the initial question I think this is a great start and we can iterate over time to improve.

@Dentrax
Copy link
Member Author

Dentrax commented Dec 15, 2022

Makes sense! We will add some test cases in this PR. (+ exit 1)

@rawlingsj
Copy link
Member

perfect - one more thing you might want to do is run make docs to generate the CLI docs.

@developer-guy
Copy link
Member

We also thought enabling/disabling options for the available linting rules might be a good option, WDYT?

Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>
@rawlingsj
Copy link
Member

We also thought enabling/disabling options for the available linting rules might be a good option, WDYT?

yeah that sounds good though maybe as a followup PR?

Copy link
Member

@rawlingsj rawlingsj left a comment

Choose a reason for hiding this comment

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

Just waiting on this and we should be good to go #2 (comment)

Signed-off-by: Furkan <furkan.turkal@trendyol.com>
Co-authored-by: Batuhan <batuhan.apaydin@trendyol.com>
@Dentrax
Copy link
Member Author

Dentrax commented Dec 15, 2022

I think we are ready for the review! @rawlingsj

  • Add some test cases (last commit)
  • Separate digest check logic to new rule: valid-pipeline-fetch-digest
  • --list option to list all available rules
  • Error out and exit 1 if any failing rule

@rawlingsj rawlingsj merged commit 8980f75 into wolfi-dev:main Dec 15, 2022
@rawlingsj
Copy link
Member

@Dentrax @developer-guy great work!

@rawlingsj
Copy link
Member

rawlingsj commented Dec 15, 2022

I'll look at getting wolfi os using the new linter and see how we go.

@Dentrax
Copy link
Member Author

Dentrax commented Dec 15, 2022

Thanks for such prompt merge! Looking forward to the new rules! 🚀

@developer-guy
Copy link
Member

Most of the development effort belongs to @Dentrax, I just added couple things, so thx @Dentrax 🏅

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.

3 participants