Skip to content
This repository has been archived by the owner on Jun 6, 2024. It is now read-only.

Add linter to the project #240

Closed
dimabru opened this issue Oct 26, 2021 · 10 comments · Fixed by #249
Closed

Add linter to the project #240

dimabru opened this issue Oct 26, 2021 · 10 comments · Fixed by #249
Assignees
Labels
enhancement New feature or request WIP Working in progress (will be deployed soon)

Comments

@dimabru
Copy link
Contributor

dimabru commented Oct 26, 2021

Is your feature request related to a problem? Please describe.
As our comminity grows, we need to setup development standards. Adding a linter is a good start

Describe the solution you'd like
A go linter (should be widely used in several big open-source projects) that corresponds with golang development best practices. Preferably not too aggressive

Additional context
Add linter to the build flow in travis as part of the deployment process

@dimabru dimabru added the enhancement New feature or request label Oct 26, 2021
@kevholmes
Copy link
Contributor

kevholmes commented Oct 26, 2021

I could take this on.

What are your thoughts on adding the linter(s) to GitHub Actions so the integration with your PR workflow is more seamless? A PR can have each linter run as a separate check to give code owners and the PR submitter more visibility into potential issues with their work and collab in a single workspace. This way when all checks/tests are passing in GH Actions you can feel more comfortable merging into main and then let Travis generate the actual build as the final step.

This way you get this kinda view in a PR as a code-submitter:
pr_view_of_checks

@dimabru
Copy link
Contributor Author

dimabru commented Oct 26, 2021

Interesting
In other words, seperate the ci from the cd.
Is there a way that a commit can pass a github action check and still make it to travis?

@kevholmes
Copy link
Contributor

Hey @dimabru Yep - we would split them apart. You could have the Actions (ci checks) trigger only on pull_request but still run other tools like Travis alongside them.

@dimabru
Copy link
Contributor Author

dimabru commented Oct 26, 2021

@romanlab thoughts?

@romanlab
Copy link
Member

@dimabru I don't have any objections. It's very different from the way we work now but may give more visibility to contributors. Doe that mean we won't run a lint phase in our Travis build?

@dimabru
Copy link
Contributor Author

dimabru commented Oct 26, 2021

Yep

@eyarz eyarz added the WIP Working in progress (will be deployed soon) label Oct 27, 2021
@kevholmes
Copy link
Contributor

I'll get started on it, thanks for being open to trying Actions out for this!

@kevholmes
Copy link
Contributor

I'm interested in the team's thoughts on perhaps including pre-commit as part of this. We could include a .pre-commit-hooks.yaml in the root of the datree repo. We could call various CI tools such as linters or other analysis tools depending on what changes have been made (we'll call a markdown linter for **.md changes in a commit) to give code contributors who want to use pre-commit the ability to get feedback sooner (on their workstation) rather than from having to sit and wait for a CI tool like Actions or Travis to run those CI tools for them on a server. The ultimate goal here is reducing the feedback loop on the CI tooling.

What's also nice about using pre-commit here is that the CI tasks running locally or on a cloud server will run the exact same tools pinned to the same versions. This way it's very much: if it runs on your system it will pass the CI linters in the cloud. There's a CI-aware nature to pre-commit as well which works great in GitHub Actions.

Pre-commit can also be used to call a project's Makefile targets where future functionality can be added in. For instance, validating that doc generation or generated code is correctly committed via pre-commit CI check that runs those gen tasks and ensures there is no diff in the commit (a feature baked into pre-commit as it will exit non-zero if a hook changes a commit's files.)

The reason I'm bringing this up is I'm also working on #217 right now and there's some good cohesion between these two issues.

Thoughts @dimabru @romanlab?

@dimabru
Copy link
Contributor Author

dimabru commented Oct 28, 2021

Hi @kevholmes, thanks for the very informative explanation.
In general, we're very open to changes that will potentially improve the project.
However, since the project is still very small in size, I'm worried its a bit pre-mature. We don't have a large enough community yet to support a lot of dev-tools for contributers.

Can you share another project that uses the same pre-commit practice you described?

@kevholmes
Copy link
Contributor

Hi @kevholmes, thanks for the very informative explanation. In general, we're very open to changes that will potentially improve the project. However, since the project is still very small in size, I'm worried its a bit pre-mature. We don't have a large enough community yet to support a lot of dev-tools for contributers.

Can you share another project that uses the same pre-commit practice you described?

Makes sense. I don't really know if many public repos are utilizing pre-commit this way, but I have been implementing it in a similar manner for clients looking to add a suite of analysis tools to their projects to help improve quality of contributions. I'll focus on adding an Action with a tool like like golang-ci that can utilize a number of Go analysis tools and is popular with other open source projects.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request WIP Working in progress (will be deployed soon)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants