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

Migrate local and CI linting and formatting to trunk.io #6564

Merged
merged 22 commits into from
Nov 6, 2023

Conversation

surovic
Copy link
Contributor

@surovic surovic commented Aug 21, 2023

Trunk is a superlinter that supports both local and CI linting and source formatting tasks. AFAIK it supports all the languages and markups that we use. The PR migrates all of our linter configuration to be compatible with trunk and also replaces flake8 with ruff as it subsumes it and is more performant.

The one caveat is that we now effectively only lint python v3.8. mypy is set to that version and ruff will only lint issues and suggest fixes that are compatible with python 3.8. and up.

The linters also only check changed code for issues. This means developers should not be harassed about issues they didn't cause.

The last side-effect of this PR is that the top level Makefile has been rewritten and now supports:

  • make lint - locally runs trunk check, which is equivalent to the CI linting workflow.
  • make format - locally runs trunk fmt, which formats changes made in the repo.
  • make docker - build the trailofbits/polytracker docker image.
  • make test - runs the pytest /polytracker/tests command in trailofbits/polytracker docker image container.
  • make clean - deletes all trailofbits/polytracker* docker images.

@surovic surovic changed the title Create a single source of code formatting truth Migrate local and CI linting and formatting to trunk Aug 23, 2023
@surovic surovic marked this pull request as ready for review August 23, 2023 12:55
@surovic surovic self-assigned this Aug 23, 2023
@surovic surovic added CI/CD Continuous Integration / Continuous Delivery developer-friendliness Related to ease of development labels Aug 23, 2023
@surovic surovic changed the title Migrate local and CI linting and formatting to trunk Migrate local and CI linting and formatting to trunk.io Aug 23, 2023
Copy link
Collaborator

@hbrodin hbrodin left a comment

Choose a reason for hiding this comment

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

This looks good to me. Nice that you did some cleanup of the readme and scripts. It looks much cleaner. With that said, I'd like for someone else to have a second opinion on this before proceeding. I have never used trunk.io before so I couldn't really tell if there are better ways of achieving what we do here.

The one thing that comes to mind is that if we now test/lint using python 3.8 we should mention that in the readme. I couldn't find anything about that.

@kaoudis
Copy link
Collaborator

kaoudis commented Sep 27, 2023

The linting multiple python versions was a legacy thing that I kept because we were already implicitly doing it! I don't think it added any value. Thanks! ✅

@kaoudis
Copy link
Collaborator

kaoudis commented Sep 27, 2023

We should probably still run tests with a couple Python versions though just in case.

@surovic
Copy link
Contributor Author

surovic commented Oct 20, 2023

We should probably still run tests with a couple Python versions though just in case.

Hmm, should we really? We essentially dictate the python version in the docker image build.

@kaoudis
Copy link
Collaborator

kaoudis commented Nov 6, 2023

We should probably still run tests with a couple Python versions though just in case.

Hmm, should we really? We essentially dictate the python version in the docker image build.

My reasoning here is that we have in the past allowed the use of different Python versions with Polytracker. If someone out there is using it with a prior Python version, it would be good to not arbitrarily take that support away from them. However, we only support Polytracker usage in the container, to the best of my knowledge, and as long as we're clear there's one true way to run Polytracker in the README, I don't see any reason to continue running builds or tests under any Pythons other than the Python version we prefer to build and run with.

@surovic surovic merged commit 077d71a into master Nov 6, 2023
9 checks passed
@surovic surovic deleted the fix_formatting_scripts branch November 6, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD Continuous Integration / Continuous Delivery developer-friendliness Related to ease of development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants