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

[LSC] Convert all Lints to CertificateLints #767

Merged
merged 2 commits into from
Nov 19, 2023

Conversation

aaomidi
Copy link
Contributor

@aaomidi aaomidi commented Nov 12, 2023

Partially addresses #765

Using the code on: https://github.com/aaomidi/zlint-migration, I've converted all lint.RegisterLint to lint.RegisterCertificateLint.

To make it easier for reviewers: look at the code here: https://github.com/aaomidi/zlint-migration, re-run this on the lints, it should create no diff compared to this PR.

Note that I've reviewed every file manually as well just in case, and I didn't spot anything.

@aaomidi aaomidi marked this pull request as ready for review November 12, 2023 19:22
@aaomidi aaomidi changed the title Convert all Lints to CertificateLints [LSC] Convert all Lints to CertificateLints Nov 12, 2023
@robplee
Copy link
Contributor

robplee commented Nov 13, 2023

I'm not sure how one would do this exactly, but I wonder if we could/should add something to the build process to try to make it impossible to add new lint.Lints or lint.LintInterfaces in PRs? Something on a similar line to Chris' magic that blocks people from merging changes to genTestCerts.go but that works everywhere?

I say this because I'm pretty sure you're going to have to fix some lints with this PR that I've had merged in since #699 was merged so I think there might be value in having a good solution that isn't "We'll just review it better" because there's enough stuff to check with zlint PRs to "just" make sure they're accurately implementing the rule they're adding. This feels like something we probably can check automatically so we probably should try.

@aaomidi
Copy link
Contributor Author

aaomidi commented Nov 13, 2023

I can actually extend the code I wrote to actively look for that in the CI step (?)

I probably don't want to include it in this PR though but I'll make a separate PR for this.

@aaomidi
Copy link
Contributor Author

aaomidi commented Nov 16, 2023

Are we waiting on anything else for this PR?

@cardonator
Copy link
Contributor

I meant to try to use these changes in our build environment to make sure they wouldn't have any immediate effect on our usage. I was wondering if this change is significant enough that we should consider incrementing the version. Not sure if there is anything else blocking.

@aaomidi
Copy link
Contributor Author

aaomidi commented Nov 16, 2023

I think this change should be invisible for most folk. Good point though it makes sense to leave it ready for a few folks to test it out on their environment.

Also if this change does accidentally break anyone, we should definitely roll it back.

@aaomidi
Copy link
Contributor Author

aaomidi commented Nov 16, 2023

@robplee: I have the check you requested #770.

For maintainers: please let me know if you'd prefer the detection code to be part of this PR instead.

@cardonator
Copy link
Contributor

Alright, I did some high level testing against this PR in our integration and confirmed that there were no issues for our use case. Ours is definitely not super complicated. I think the second step you described above would definitely break us so we should probably make sure we can cut a new release prior to that to give time for people to migrate over to the new interfaces. 👍

@aaomidi
Copy link
Contributor Author

aaomidi commented Nov 16, 2023

Thank you so much for testing this change on your end!

@zakird
Copy link
Member

zakird commented Nov 16, 2023

This looks reasonable to me. @christopher-henderson would you also be willing to take a look?

@christopher-henderson
Copy link
Member

I'm not sure how one would do this exactly, but I wonder if we could/should add something to the build process to try to make it impossible to add new lint.Lints or lint.LintInterfaces in PRs? Something on a similar line to Chris' magic that blocks people from merging changes to genTestCerts.go but that works everywhere?

Indeed, doing so is possible (it parses the AST of lints and does something similar to what @aaomidi's code migrator is doing).

I'll be able to take this on (pending the conversation on implementing the breaking changes), as well as some repo cleanup activities regarding lint templating being slightly off.

Copy link
Member

@christopher-henderson christopher-henderson 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 @aaomidi for automating this transition!

@christopher-henderson christopher-henderson merged commit 45e6204 into zmap:master Nov 19, 2023
4 checks passed
@robplee robplee mentioned this pull request Dec 5, 2023
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.

5 participants