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

Custom static analysis tooling for CI/CD #551

Merged
merged 13 commits into from
Feb 15, 2021
Merged

Custom static analysis tooling for CI/CD #551

merged 13 commits into from
Feb 15, 2021

Conversation

christopher-henderson
Copy link
Member

@christopher-henderson christopher-henderson commented Jan 24, 2021

I'm still working on seeing if it works with CI/CD, plus it needs more code docs, but this is the general idea.

At the very least it has found several lints that we missed from #536 !

Found 2 linting errors
--------------------
Linting Error

Got the wrong method signature as the first function declaration within the linter.
ZLint lints must have func init() { ... } as their first function declaration

File ../../lints/cabf_br/lint_cab_dv_conflicts_with_locality.go, line 28

func (l *certPolicyConflictsWithLocality) Initialize() error {
	return nil
}

For more information, please see the following citations.
	https://github.com/zmap/zlint/issues/371
	https://golang.org/doc/effective_go.html#init

--------------------
Linting Error

Got the wrong method signature as the first function declaration within the linter.
ZLint lints must have func init() { ... } as their first function declaration

File ../../lints/rfc/lint_subject_printable_string_badalpha.go, line 39

func validatePrintableString(rawPS []byte) error {
	if !printableStringRegex.Match(rawPS) {
		return errors.New("encoded PrintableString contained illegal characters")
	}
	return nil
}

For more information, please see the following citations.
	https://github.com/zmap/zlint/issues/371
	https://golang.org/doc/effective_go.html#init

This addresses issue #546

Copy link
Contributor

@sleevi sleevi left a comment

Choose a reason for hiding this comment

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

Deferring this to the real go users, so just one inline comment from mucking about with GitHub's CI tooling ;)

v3/integration/lints/lint/lint.go Show resolved Hide resolved
@cpu cpu self-assigned this Jan 25, 2021
@cpu
Copy link
Member

cpu commented Feb 1, 2021

Haven't forgotten about reviewing this branch. I'll get to it this week 🤞

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Thank you for taking this @christopher-henderson. It's a really cool piece of work. I haven't worked with the Go AST before and I'm impressed that you got something workable up and running so quickly.

I left a lot of comments but most of them are very small nits. The core of the work seems great 🚀

v3/integration/lints/go.mod Outdated Show resolved Hide resolved
v3/integration/lints/go.mod Outdated Show resolved Hide resolved
v3/integration/lints/lint/lint.go Show resolved Hide resolved
v3/integration/lints/lint/lint.go Show resolved Hide resolved
v3/integration/lints/lint/lint.go Outdated Show resolved Hide resolved
v3/integration/lints/main.go Outdated Show resolved Hide resolved
v3/integration/lints/main.go Outdated Show resolved Hide resolved
v3/integration/lints/main.go Outdated Show resolved Hide resolved
v3/integration/lints/main_test.go Outdated Show resolved Hide resolved
v3/integration/lints/utils/filters.go Outdated Show resolved Hide resolved
@cpu
Copy link
Member

cpu commented Feb 3, 2021

At the very least it has found several lints that we missed from #536 !

Also: that rules! Nice work. It's gratifying to see having this as part of CI will definitely result in greater consistency.

@christopher-henderson
Copy link
Member Author

Hey, the linter successfully failed! Reckon I should fix those while we're here.

@christopher-henderson
Copy link
Member Author

I haven't worked with the Go AST before and I'm impressed that you got something workable up and running so quickly.

😊 I admit it's not my first rodeo with the Go compiler. I actually forked it (GoSearch) for my grad school research in order to make a dialect of Go that supported a backtracking engine at runtime.

@cpu
Copy link
Member

cpu commented Feb 8, 2021

I admit it's not my first rodeo with the Go compiler. I actually forked it (GoSearch) for my grad school research in order to make a dialect of Go that supported a backtracking engine at runtime.

@christopher-henderson That's very cool :-) Thanks for sharing.

Is this branch ready for me to review again or do you need more time to land changes for the feedback?

@christopher-henderson christopher-henderson force-pushed the metalinter branch 3 times, most recently from aa3f9ec to 02a4e04 Compare February 9, 2021 21:50
@christopher-henderson
Copy link
Member Author

christopher-henderson commented Feb 9, 2021

@cpu I believe it should be GTG now.

Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Thanks @christopher-henderson, I mostly flagged typos in the new docstrings this pass. If you can fix those and the other couple things I commented on I think this is ready to be stamped/merged.

v3/integration/lints/filters/nodes.go Outdated Show resolved Hide resolved
v3/integration/lints/filters/nodes.go Outdated Show resolved Hide resolved
v3/integration/lints/lint/lint.go Outdated Show resolved Hide resolved
v3/integration/lints/lint/lint.go Outdated Show resolved Hide resolved
v3/integration/lints/lint/lint.go Outdated Show resolved Hide resolved
v3/integration/lints/lint/lint.go Outdated Show resolved Hide resolved
v3/integration/lints/main.go Outdated Show resolved Hide resolved
v3/integration/lints/main.go Outdated Show resolved Hide resolved
v3/makefile Show resolved Hide resolved
v3/makefile Show resolved Hide resolved
Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

@zakird zakird merged commit 835500b into master Feb 15, 2021
@zakird zakird deleted the metalinter branch February 15, 2021 18:58
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.

4 participants