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

CI: Automatically committing/suggesting linter fixes for PRs #2925

Closed
terriko opened this issue Apr 19, 2023 · 2 comments · Fixed by #3017
Closed

CI: Automatically committing/suggesting linter fixes for PRs #2925

terriko opened this issue Apr 19, 2023 · 2 comments · Fixed by #3017
Labels
CI Related to our continuous integration service (GitHub Actions)

Comments

@terriko
Copy link
Contributor

terriko commented Apr 19, 2023

We currently run a whole series of linters on every PR:

https://github.com/intel/cve-bin-tool/blob/main/CONTRIBUTING.md#running-linters

Some of them (black, isort) can automatically fix issues they find, but in our current workflow we have to ask the user to run them and update the PR (although occasionally if the contributor is having trouble or it's really obvious to me where the flaw is, I'll either set up a "suggestion" or update the PR directly to fix issues).

On one hand, this little stepping stone usually helps new contributors get a development environment set up and helps them learn about the tools we use. On the other hand, it can mean a full day before code is merged because I have to leave a comment requesting changes. If the tools can auto-fix why don't we have a bot that does it so both the contributor and reviewer can spend more time on the parts of coding that actually need a human?

So... I'm opening this issue for people to discuss two things:

  1. Is this a thing we want?
    • I'm leaning towards yes, but I'd appreciate some feedback from folk who've done their first commits more recently)
  2. If we do want to auto-sort and blacken code, how should it be implemented? (e.g. what tools, should it suggest and comment, update the PR directly, etc?)
@terriko terriko added the CI Related to our continuous integration service (GitHub Actions) label Apr 19, 2023
@metabiswadeep
Copy link
Contributor

@terriko Since there are no comments that indicate any benefits of not using the auto update in the linter, should I go ahead and work on this?

@terriko
Copy link
Contributor Author

terriko commented Apr 24, 2023

If by "work on it" you mean "investigate some options and post a couple of viable solutions here so people can discuss them" then sure.

I've got hackathon folk working on this project through April 28 and I don't want to change the CI system in the middle of the event, so I'm not really interested in actual PRs related to this until after that ends. But the good news is that leaves us at least a week to leave this issue open for discussion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Related to our continuous integration service (GitHub Actions)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants