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

Fail CI build if golint finds problems (non-functional) #524

Closed
pedroMMM opened this issue Dec 14, 2019 · 7 comments · Fixed by #533
Closed

Fail CI build if golint finds problems (non-functional) #524

pedroMMM opened this issue Dec 14, 2019 · 7 comments · Fixed by #533

Comments

@pedroMMM
Copy link
Contributor

@pedroMMM pedroMMM commented Dec 14, 2019

Describe the feature:

Golint should find 0 problems or fail the CI build.

Describe the solution you'd like

While it's non-functional, it's worrying that golint finds a high quantity of problems during builds.
Since most problems seem to be documentation, it will double as a method to lower the entry barrier to understand Goss internals.

Describe alternatives you've considered

Some actions can be taken to minimize the issue:

  • Increasing golint problem confidence.
  • Ignoring certain types of golint problems.
  • Simply removing golint.

Go vet should also be enabled as part of the build.

@aelsabbahy

This comment has been minimized.

Copy link
Owner

@aelsabbahy aelsabbahy commented Dec 14, 2019

Agree, approved

@aelsabbahy aelsabbahy added the approved label Dec 14, 2019
@aelsabbahy

This comment has been minimized.

Copy link
Owner

@aelsabbahy aelsabbahy commented Dec 14, 2019

Somewhat related note, ever use codeclimate or tools like that? I prefer local tools myself, but wondering if in addition having codeclimate provides clearer visibility to contributors.

https://codeclimate.com/changelog/5a3a8fd40516130297001e26/

@pedroMMM

This comment has been minimized.

Copy link
Contributor Author

@pedroMMM pedroMMM commented Dec 15, 2019

I tried Code Climate recently, actually. I have used SonarQube in the past as well and adored the visibility and accountability it brought into our workflow.

I can play around with https://docs.codeclimate.com/docs/github-pull-requests on my fork and show you examples and my configuration if you like it.

In theory, Code Climate and golint overlap a lot so only one of them would be absolutely needed. I would still want to leave golint in make so it runs locally to provide quick feedback for development and the Code Climate on PR/Commit for a detailed view with history.

Plus Code Climate does Unit Test coverage report 🎉

This will take a while to get all the problems under control so I might break it into more than a single PR, was I able to through it, to keep merge conflicts at a minimum.

@pedroMMM

This comment has been minimized.

Copy link
Contributor Author

@pedroMMM pedroMMM commented Dec 17, 2019

I played around with Code Climate and I have fast proof of concept available for feedback: https://codeclimate.com/github/pedroMMM/goss/issues

As suspected Code Climate also supports sourcing problems from gofmt, golint, govet. This would enable local testing but also centralizing information on Code Climate and PR validation. The big issue here is that with all of those sources enabled Code Climate is reporting 432 issues. Fixing all of this would be an insane PR!

I suggest evaluate my Code Climate concept before anything else. Then if you still want to continue down this path (which I think we should) I would create a PR and work with you to configure Code Climate on this repo. Only then should anyone start working on the cleanup, which could be scoped to packages or files to minimize confusion and even encourage the community to contribute.

@pedroMMM

This comment has been minimized.

Copy link
Contributor Author

@pedroMMM pedroMMM commented Dec 27, 2019

@aelsabbahy I am tagging you in case you have missed my last post. Let me know if you want to reduce scope or any forward steps.

@aelsabbahy

This comment has been minimized.

Copy link
Owner

@aelsabbahy aelsabbahy commented Dec 28, 2019

I agree 100% with your last post.

Definitely, should have the ability to locally test.

To be honest with you, I haven't had the time to do a thorough review on this with the holidays and all, that's why I haven't responded.

I'll go through this in detail probably sometime in the first two weeks of January.

At a high level:

  • If it looks good, I'll let you know
  • You can submit a Pr and I'll work with you on getting codeclimate working
  • Initially it can be purely informative, later on we can start setting more restrictive checks to fail the build. However, I would want this support to be local first or at least easily reproducible locally.

Happy holidays, and thanks for all your great contributions so far!

@pedroMMM

This comment has been minimized.

Copy link
Contributor Author

@pedroMMM pedroMMM commented Dec 28, 2019

That is more than fair, I am sorry if made you feel pressured. There is a lot to unpack in the Code Commit report, I was just making sure it didn't get lost.

I just want to make clear that not all the problems there are possible to analyze locally. Code Commit adds a little bit of their analysis on top of gofmt, golint, govet.

As soon as I can I will get a PR with the configuration changes and instructions on how to get Code Commit configured on your side.

@pedroMMM pedroMMM mentioned this issue Jan 2, 2020
3 of 3 tasks complete
aelsabbahy added a commit that referenced this issue Jan 20, 2020
* Add Code Coverage Support

* Add codeclimate token

Co-authored-by: Ahmed Elsabbahy <aelsabbahy@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.