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

Lint with Trunk.io #14

Merged
merged 12 commits into from
Feb 17, 2023
Merged

Lint with Trunk.io #14

merged 12 commits into from
Feb 17, 2023

Conversation

oldsj
Copy link
Contributor

@oldsj oldsj commented Feb 7, 2023

Lints with trunk check.
Example implementation at https://github.com/trailofbits/firebreak/pull/341

Repo onboarding process

  1. Install the ./trunk script to the root of your repo and check it in.
curl -LO https://trunk.io/releases/trunk
chmod +x trunk
  1. Initialize trunk with ./trunk init. This will scan your repo and generate a .trunk/trunk.yaml with all the required linters.

Linting workflow

Developers simply need to run ./trunk check from the root of the repo to get the same CI linting results locally. No need to install anything manually.

Run ./trunk fmt to fix any linting issues.
Trunk will cache and is diff aware locally so it will only run the appropriate linter and only on affected files.
There is also a VSCode extension that can be set as the default formatter https://marketplace.visualstudio.com/items?itemName=Trunk.io

Native VSCode linting
image

Trunk sidebar
image

./trunk check prompts to auto format
image

PR annotations
image

Risks / downside

The trunk binary appears to be closed-source (I can't find the source) and trunk.io is a VC-backed startup. If trunk.io goes under we can still use the linting tools that it's wrapping with a make target. We should probably provide that by default for opensource projects.

@oldsj oldsj marked this pull request as ready for review February 7, 2023 15:59
@oldsj oldsj requested a review from woodruffw February 7, 2023 15:59
@oldsj
Copy link
Contributor Author

oldsj commented Feb 7, 2023

Firebreak implementation at https://github.com/trailofbits/firebreak/pull/341

@ekilmer
Copy link

ekilmer commented Feb 7, 2023

Very interesting tool!

Do the lint config files need to live in the .trunk/config directory?

It'd be nice if there were a way to specify where the config files live in .trunk/trunk.yaml because other tools (like IDEs and editors+plugins, although I do see that the VSCode extension likely takes care of this) default to looking at known locations, like the root of the repo or current directory. It'd be nice to maintain that support out of the box. On the other hand, most of these lint tools can be configured from the CLI to use a specific file and people could reconfigure their setup to point to the file in .trunk/plugins.

@oldsj
Copy link
Contributor Author

oldsj commented Feb 7, 2023

They do not that's a good callout, this is just what ./trunk init generated. We probably want config in the root directory to keep things standards based, especially for opensource repos.

From the docs:

If you'd like, trunk also supports migrating any linter configurations from the root of your repository into a .trunk/config folder. These config files will be symlinked in during any trunk check run

@ekilmer
Copy link

ekilmer commented Feb 7, 2023

We probably want config in the root directory to keep things standards based, especially for opensource repos.

💯

These config files will be symlinked in during any trunk check run

That sounds promising!

@woodruffw
Copy link
Member

  1. Install the ./trunk script to the root of your repo and check it in.

I'm not super jazzed about having a third-party shell script in the root of all of our projects (it harkens back to the bad old days of build.sh). Is it at all possible to have trunk be installed on a per-system basis instead (e.g. brew install trunk from a third-party tap, or even just ~/bin/trunk)?

Developers simply need to run ./trunk check from the root of the repo to get the same CI linting results locally. No need to install anything manually.

Making sure I understand: does trunk check re-run all linters locally, or does it just copy the latest CI linting results locally?

I'm hoping it's the former, because IMO having to push things up to get linting results results in poor repo hygiene and will make it harder for us to make things external-contributor friendly (e.g. we'll need to make sure any trunk-based workflows also behave correctly on 3p PRs, and will potentially have to deal with a lot of PR noise from novice contributors).

Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

I'm curious to see how this will work with a C/C++ project, but I'm overall hesitant to push trunk as our standard linting/CQA tool. To summarize my position:

  1. I'm not a big fan of an ambiguously licensed shell script copied into each of our repos, both from a maintenance/hygiene perspective and from a licensing/opacity perspective.
  2. I'm worried about the long-term stability of this solution: my personal experience with VC-backed ecosystem tooling hasn't been great (lots of disappearing projects, or projects that pivot to extreme prices once they have a captive customer base). Separately (and less seriously), I'd like to make sure trunk is no longer pinning local cores the way it seemed to the last time we evaluated it 🙂
  3. We need to make sure trunk doesn't violate our client obligations: we work with clients whose source code is not public and/or can't be shared (including through third-party assistive online services), so we need to ensure that trunk doesn't indefinitely retain copies of source code via its VSCode extension or whatever server-side state it uses.
  4. As an abstract goal, I want our linting and CQA tooling to have an educational/growth component for engineers at ToB: IMO we should automate the annoying and fiddly stuff, but not at the cost of not exposing our engineers to details that will make them better overall engineers. The long term concern there then is that trunk cements a local maxima in our linting, since people will no longer be experimenting (as much) with individual tools and settings.

@woodruffw
Copy link
Member

Is it at all possible to have trunk be installed on a per-system basis instead (e.g. brew install trunk from a third-party tap, or even just ~/bin/trunk)?

Answering my own question: brew install trunk-io should work. That's a Cask, meaning that it'll be a proprietary binary, but I'm not extremely worried about that part if we aren't checking it into the repo.

@oldsj
Copy link
Contributor Author

oldsj commented Feb 7, 2023

Making sure I understand: does trunk check re-run all linters locally, or does it just copy the latest CI linting results locally?

Yes it will re-run locally, independently of CI, but local and CI do share the trunk.yaml config

@oldsj
Copy link
Contributor Author

oldsj commented Feb 7, 2023

or whatever server-side state it uses.

As far as I know there is no server-side state with this tool since we're not using their paid webapp features

@oldsj
Copy link
Contributor Author

oldsj commented Feb 16, 2023

As far as I know there is no server-side state with this tool since we're not using their paid webapp features

This isn't true they do have a webapp that trunk CI will upload results to, for a central place to view our linting status org wide
https://app.trunk.io/

@oldsj oldsj requested a review from woodruffw February 16, 2023 19:39
Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

LGTM overall, one offline comment about what we're doing with make deps etc. 🙂

@oldsj oldsj merged commit e08a803 into main Feb 17, 2023
@oldsj oldsj deleted the trunk-lint branch February 17, 2023 16:18
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