[v2] Add pre-commit and CI static checks #1025

turbaszek · 2020-08-22T10:45:17Z

Code quality is important, especially in an open source project where the code will be maintained for a long time and by multiple people. So I would like to suggest using https://pre-commit.com framework to automate a lot of stuff and assure that our code is nice.

It plays nicely with Github action and allows a plethora of checks (building TOC, license insert, liniting etc) and gives possibility to build custom ones. For example here's configuration from Apache Airflow:
https://github.com/apache/airflow/blob/master/.pre-commit-config.yaml

Checklist

Commits are signed with Developer Certificate of Origin (DCO)
Tests have been added
A PR is opened to update the documentation on https://github.com/kedacore/keda-docs
Changelog has been updated

turbaszek · 2020-08-22T11:00:50Z

There are quite a few go-lint errors that I'm happy to fix if we agree to proceed with this change 😃

turbaszek · 2020-08-22T20:16:17Z

.pre-commit-config.yaml

+      exclude: |
+        (?x)(
+          .*zz_generated.*|
+          ^pkg/apis/keda/v1alpha1/triggerauthentication_types\.go$|
+          ^pkg/handler/hashicorpvault_handler\.go$|
+          ^pkg/scalers/azure/azure_aad_podidentity\.go$|
+          ^pkg/scalers/azure/azure_eventhub\.go$|
+          ^pkg/scalers/azure/azure_monitor\.go$|
+          ^pkg/scalers/azure/azure_storage\.go$|
+          ^pkg/scalers/azure_eventhub_scaler\.go$|
+          ^pkg/scalers/azure_queue_scaler\.go$|
+          ^pkg/scalers/azure_servicebus_scaler\.go$|
+          ^pkg/scalers/cron_scaler\.go$|
+          ^pkg/scalers/kafka_scram_client\.go$|
+          ^pkg/scalers/liiklus_scaler\.go$|
+          ^pkg/scalers/postgresql_scaler\.go$|
+          ^pkg/scalers/prometheus\.go$|
+          ^pkg/scalers/rabbitmq_scaler\.go$|
+          ^pkg/scalers/scaler\.go$|
+          ^version/version\.go$
+        )


Those can be fixed in follow up PRs and make good candidates for good-first-issues (Hacktober is coming) 😄

If this gets merged, please don't forget to open issues for these.

tomkerkhove

This looks OK to me, thanks!

Just make sure to target v2 instead of master so that @zroubalik doesn't have to plow through all of these.

Just a tip, let's discuss this in an issue first in the future because if we didn't like this then you would've wasted a ton of work

tomkerkhove · 2020-08-24T05:25:17Z

.pre-commit-config.yaml

+      exclude: |
+        (?x)(
+          .*zz_generated.*|
+          ^pkg/apis/keda/v1alpha1/triggerauthentication_types\.go$|
+          ^pkg/handler/hashicorpvault_handler\.go$|
+          ^pkg/scalers/azure/azure_aad_podidentity\.go$|
+          ^pkg/scalers/azure/azure_eventhub\.go$|
+          ^pkg/scalers/azure/azure_monitor\.go$|
+          ^pkg/scalers/azure/azure_storage\.go$|
+          ^pkg/scalers/azure_eventhub_scaler\.go$|
+          ^pkg/scalers/azure_queue_scaler\.go$|
+          ^pkg/scalers/azure_servicebus_scaler\.go$|
+          ^pkg/scalers/cron_scaler\.go$|
+          ^pkg/scalers/kafka_scram_client\.go$|
+          ^pkg/scalers/liiklus_scaler\.go$|
+          ^pkg/scalers/postgresql_scaler\.go$|
+          ^pkg/scalers/prometheus\.go$|
+          ^pkg/scalers/rabbitmq_scaler\.go$|
+          ^pkg/scalers/scaler\.go$|
+          ^version/version\.go$
+        )


If this gets merged, please don't forget to open issues for these.

.github/workflows/static-checks.yml

turbaszek · 2020-08-24T07:11:26Z

@tomkerkhove thanks for the review! Regarding:

Just a tip, let's discuss this in an issue first in the future because if we didn't like this then you would've wasted a ton of work

I just copy pasted pre-commit config from my other go repo so not a lot of work 😉

turbaszek · 2020-08-24T07:17:23Z

Oh, DCO doesn't accept PR added from GitHub review? :<

turbaszek · 2020-08-24T08:48:50Z

@zroubalik I've rebased this change onto v2 👍

.pre-commit-config.yaml

mik-laj · 2020-08-24T21:00:22Z

❤️ 😻 ❤️

CONTRIBUTING.md

tomkerkhove

LGTM but I'll leave it up to @zroubalik to give the sign-off

tomkerkhove · 2020-08-25T05:34:21Z

CONTRIBUTING.md

@@ -4,6 +4,19 @@ Thanks for helping make KEDA better 😍.

 There are many areas we can use contributions - ranging from code, documentation, feature proposals, issue triage, samples, and content creation.

+<!-- START doctoc generated TOC please keep comment here to allow auto update -->


That's great, but how does this work? Should add some docs on this :)

I would say that's enough to google doctoc to find out more information. Also not sure where to put such docs

Yeah. And wit pre-commits you basically do not have to worry about it - just install pre-commit and you forget about it..

That's a bit of an odd argument as that would mean we don't have to doc the pre-commit thingy but it's ok.

Yeah. And wit pre-commits you basically do not have to worry about it - just install pre-commit and you forget about it..

We tend to make it easy for folks to contribute so you cannot assume everybody has pre-commit installed, that's why we have a CI.

We tend to make it easy for folks to contribute so you cannot assume everybody has pre-commit installed, that's why we have a CI.

As we have totally lightweight pre-commit setup we should probably encourage people to use them. It's good to teach people how to automate stuff and focus on the most important things like the code itself and documentation. However, that's only my personal opinion.

That's definitely the case and I agree, but we can't expect everyone to use it.

We also have it integrated on the CI, so if someone does not use pre-commit locally, they will get a readable diff on the CI that will show what changes should be made.

Yes, that was my point on #1025 (comment) indeed

Yeah. Pre-commits are working like a charm on Apache Airflow for > 1.5 year now and we have plenty of pre-commits running/ You can see it working here for example: https://github.com/apache/airflow/runs/1026196376?check_suite_focus=true#step:7:108 - it integrates very well with CI. you simply run pre-commit run --all-files in the CI and you get the very same checks executed in CI as locally.

And as @mik-laj mentioned - with --show-diff-on-failure flag it will even show you what modification would be made if you run pre-commit locally. In case it fails in CI it will even tell you what to do 'pip install pre-commit' and then pre-commit run <id of pre-commit that failed>. But you still can fix it manually if you prefer (though a lot of pre-commit checks can fix things for you automatically (like end-of-file checking, sorting import in python etc.) and then the only thing you have to do you have to commit modified files. And you have literally hundreds of hooks that you can start using.

So you do not have to even install it as pre-commit (though it is super-easy pre-commit install will do the job after installit it with pip. The nice thing about it is that you simply forget about it once you install - it will automatically run all the precommits that were added by others since and you will actually fix the problems even before others see them.

I do not see any drawbacks of using pre-commit to be honest if your project's goal is to keep some consistency, it's the perfect tool for the job IMHO.

Don't get me wrong, I'm not saying it's a bad thing or so; I was just saying that it's fine and the CI will cover it but we shouldn't force people to use it.

I'm not saying you are, it was just a general remark :) This escalated quickly.

CONTRIBUTING.md

Signed-off-by: Tomek Urbaszek <tomasz.urbaszek@polidea.com>

README.md

ahmelsayed

change looks good to me. I'm assuming the CI pre-commit will fail if something needs to be fixed, right? I haven't used pre-commit before but looks neat.

Signed-off-by: Tomek Urbaszek <tomasz.urbaszek@polidea.com>

turbaszek · 2020-08-25T20:44:22Z

I'm assuming the CI pre-commit will fail if something needs to be fixed, right? I haven't used pre-commit before but looks neat.

That's right! For example:

Check if scalers are sorted in scaler_handler.go............................Failed
hookid: sort-scalers

Scalers are not sorted alphabetical in pkg/scaling/scale_handler.go

Most of pre-commit hooks we use apply the changes automatically during git commit (remove white spaces, go-fmt, go-lint) so not to much to do for developers

turbaszek · 2020-08-26T08:53:18Z

@tomkerkhove @zroubalik are we good to go?

tomkerkhove

LGTM, just one more change.

tomkerkhove · 2020-08-26T08:57:09Z

CONTRIBUTING.md

@@ -4,6 +4,19 @@ Thanks for helping make KEDA better 😍.

 There are many areas we can use contributions - ranging from code, documentation, feature proposals, issue triage, samples, and content creation.

+<!-- START doctoc generated TOC please keep comment here to allow auto update -->


Don't get me wrong, I'm not saying it's a bad thing or so; I was just saying that it's fine and the CI will cover it but we shouldn't force people to use it.

I'm not saying you are, it was just a general remark :) This escalated quickly.

CONTRIBUTING.md

zroubalik

LGTM, just minor remark on "enforcing" pre-commit. Thanks for the contribution

CONTRIBUTING.md

Signed-off-by: Tomek Urbaszek <tomasz.urbaszek@polidea.com>

zroubalik

LGTM

mik-laj · 2020-08-26T11:08:52Z

Tomek, great work! Thanks a lot! 🎉

turbaszek · 2020-08-26T11:14:16Z

Thanks all! 🚀

potiuk · 2020-08-26T17:22:05Z

❤️

kaxil · 2020-08-26T17:29:05Z

<3

Signed-off-by: Neelanjan Manna <neelanjan.manna@harness.io> Signed-off-by: Neelanjan Manna <neelanjan.manna@harness.io>

turbaszek marked this pull request as ready for review August 22, 2020 20:15

turbaszek requested review from ahmelsayed and zroubalik as code owners August 22, 2020 20:15

turbaszek commented Aug 22, 2020

View reviewed changes

tomkerkhove reviewed Aug 24, 2020

View reviewed changes

turbaszek changed the base branch from master to v2 August 24, 2020 08:34

turbaszek force-pushed the add-pre-commits-ci branch 2 times, most recently from 5990e57 to 8010885 Compare August 24, 2020 08:47

turbaszek force-pushed the add-pre-commits-ci branch 2 times, most recently from 25595ec to 25f8027 Compare August 24, 2020 08:52

zroubalik changed the title ~~Add pre-commit and CI static checks~~ [v2] Add pre-commit and CI static checks Aug 24, 2020

turbaszek requested a review from tomkerkhove August 24, 2020 15:35

turbaszek mentioned this pull request Aug 24, 2020

[v2] Add MetricsAPIScaler #1026

Merged

8 tasks

turbaszek commented Aug 24, 2020

View reviewed changes

.pre-commit-config.yaml Show resolved Hide resolved

mik-laj reviewed Aug 24, 2020

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

tomkerkhove reviewed Aug 25, 2020

View reviewed changes

potiuk approved these changes Aug 25, 2020

View reviewed changes

turbaszek mentioned this pull request Aug 25, 2020

[v2] add details on creating a new scaler #1038

Merged

turbaszek added 5 commits August 25, 2020 15:05

Add pre-commit and CI static checks

916517f

Signed-off-by: Tomek Urbaszek <tomasz.urbaszek@polidea.com>

Check if scalers are sorted and check offensive language

381fa7d

Signed-off-by: Tomek Urbaszek <tomasz.urbaszek@polidea.com>

Add autogenreated tables of content

8be7c75

Signed-off-by: Tomek Urbaszek <tomasz.urbaszek@polidea.com>

Improve information about pre-commits in KEDA

3abbd50

Signed-off-by: Tomek Urbaszek <tomasz.urbaszek@polidea.com>

Align casing

dfbd618

Signed-off-by: Tomek Urbaszek <tomasz.urbaszek@polidea.com>

turbaszek force-pushed the add-pre-commits-ci branch from cef337d to dfbd618 Compare August 25, 2020 13:05

ahmelsayed reviewed Aug 25, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

ahmelsayed reviewed Aug 25, 2020

View reviewed changes

TobKed approved these changes Aug 25, 2020

View reviewed changes

Fix merge conflict

7acc1cf

Signed-off-by: Tomek Urbaszek <tomasz.urbaszek@polidea.com>

mik-laj approved these changes Aug 25, 2020

View reviewed changes

tomkerkhove approved these changes Aug 26, 2020

View reviewed changes

zroubalik approved these changes Aug 26, 2020

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

fixup! Fix merge conflict

c1bf8d2

Signed-off-by: Tomek Urbaszek <tomasz.urbaszek@polidea.com>

turbaszek requested review from zroubalik and tomkerkhove August 26, 2020 10:52

zroubalik approved these changes Aug 26, 2020

View reviewed changes

tomkerkhove merged commit 02f6da7 into kedacore:v2 Aug 26, 2020

This was referenced Aug 28, 2020

Make api/v1alpha1 revive compatible #1054

Closed

Make controllers/ golint compatible #1055

Closed

Make pkg/scalers revive compatible #1056

Closed

Make pkg/scaling revive compatible #1057

Closed

Make pkg/util golint compatible #1058

Closed

SpiritZhou pushed a commit to SpiritZhou/keda that referenced this pull request Jul 18, 2023

updates grammar in scaling-deployments docs (kedacore#1025)

1885ca3

Signed-off-by: Neelanjan Manna <neelanjan.manna@harness.io> Signed-off-by: Neelanjan Manna <neelanjan.manna@harness.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v2] Add pre-commit and CI static checks #1025

[v2] Add pre-commit and CI static checks #1025

turbaszek commented Aug 22, 2020 •

edited

Loading

turbaszek commented Aug 22, 2020

turbaszek Aug 22, 2020 •

edited

Loading

tomkerkhove Aug 24, 2020

tomkerkhove left a comment

tomkerkhove Aug 24, 2020

turbaszek commented Aug 24, 2020

turbaszek commented Aug 24, 2020 •

edited

Loading

turbaszek commented Aug 24, 2020

mik-laj commented Aug 24, 2020 •

edited

Loading

tomkerkhove left a comment

tomkerkhove Aug 25, 2020

turbaszek Aug 25, 2020

potiuk Aug 25, 2020

tomkerkhove Aug 25, 2020

turbaszek Aug 25, 2020

tomkerkhove Aug 25, 2020

mik-laj Aug 25, 2020

tomkerkhove Aug 25, 2020

potiuk Aug 25, 2020 •

edited

Loading

tomkerkhove Aug 26, 2020

ahmelsayed left a comment

turbaszek commented Aug 25, 2020

turbaszek commented Aug 26, 2020

tomkerkhove left a comment

tomkerkhove Aug 26, 2020

zroubalik left a comment

zroubalik left a comment

mik-laj commented Aug 26, 2020 •

edited

Loading

turbaszek commented Aug 26, 2020

potiuk commented Aug 26, 2020

kaxil commented Aug 26, 2020

		@@ -4,6 +4,19 @@ Thanks for helping make KEDA better 😍.

		There are many areas we can use contributions - ranging from code, documentation, feature proposals, issue triage, samples, and content creation.

		<!-- START doctoc generated TOC please keep comment here to allow auto update -->

[v2] Add pre-commit and CI static checks #1025

[v2] Add pre-commit and CI static checks #1025

Conversation

turbaszek commented Aug 22, 2020 • edited Loading

Checklist

turbaszek commented Aug 22, 2020

turbaszek Aug 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomkerkhove left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turbaszek commented Aug 24, 2020

turbaszek commented Aug 24, 2020 • edited Loading

turbaszek commented Aug 24, 2020

mik-laj commented Aug 24, 2020 • edited Loading

tomkerkhove left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmelsayed left a comment

Choose a reason for hiding this comment

turbaszek commented Aug 25, 2020

turbaszek commented Aug 26, 2020

tomkerkhove left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zroubalik left a comment

Choose a reason for hiding this comment

zroubalik left a comment

Choose a reason for hiding this comment

mik-laj commented Aug 26, 2020 • edited Loading

turbaszek commented Aug 26, 2020

potiuk commented Aug 26, 2020

kaxil commented Aug 26, 2020

turbaszek commented Aug 22, 2020 •

edited

Loading

turbaszek Aug 22, 2020 •

edited

Loading

turbaszek commented Aug 24, 2020 •

edited

Loading

mik-laj commented Aug 24, 2020 •

edited

Loading

potiuk Aug 25, 2020 •

edited

Loading

mik-laj commented Aug 26, 2020 •

edited

Loading