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

chore: Move pre-commit config to the top level of kedro-plugins #259

Merged
merged 42 commits into from
Jul 19, 2023

Conversation

SajidAlamQB
Copy link
Contributor

@SajidAlamQB SajidAlamQB commented Jul 5, 2023

Description

related to: kedro-org/kedro#2337

Pre-commit configuration is maintained per-plugin so we should move pre-commit config to the top level and use path filters if/when necessary.

Not ideal because part of the idea of a monorepo is that configuration for consistent styling, etc. can be shared.

Development notes

I've been running into several issues with pre-commit, most stem from the fact that we can't dynamically run the hooks depending on the plugin environment we are in on the CI. Pre-commit runs every hook resulting in errors appearing in test environments not set up for a certain plugin.

Update:

Updated make lint it will now run each individual pre-commit hook with && the drawback from this is that it will stop running if one hook fails. The alternative is to use ; but CI won't detect if a hook fails so I've gone with &&. This also means we can go back to using make plugin=${{ inputs.plugin }} lint in check-plugins.yml.

lint:
	pre-commit run trailing-whitespace --all-files && pre-commit run end-of-file-fixer --all-files && pre-commit run check-yaml --all-files && pre-commit run check-added-large-files --all-files && pre-commit run check-case-conflict --all-files && pre-commit run check-merge-conflict --all-files && pre-commit run debug-statements --all-files && pre-commit run requirements-txt-fixer --all-files && pre-commit run flake8 --all-files && pre-commit run isort-$(plugin) --all-files --hook-stage manual && pre-commit run black-$(plugin) --all-files --hook-stage manual && pre-commit run secret_scan --all-files --hook-stage manual && pre-commit run bandit --all-files --hook-stage manual && pre-commit run pylint-$(plugin) --all-files --hook-stage manual && pre-commit run pylint-$(plugin)-features --all-files --hook-stage manual && pre-commit run pylint-$(plugin)-tests --all-files --hook-stage manual

Checklist

  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the relevant RELEASE.md file
  • Added tests to cover my changes

@SajidAlamQB SajidAlamQB self-assigned this Jul 5, 2023
@SajidAlamQB SajidAlamQB changed the title Move pre-commit config to the top level of kedro-plugins chore: Move pre-commit config to the top level of kedro-plugins Jul 5, 2023
@SajidAlamQB
Copy link
Contributor Author

I've updated the make lint command to ignore import errors when running pylint in our CI environment. This modification is due to a limitation of pre-commit, which doesn't have a mechanism to dynamically choose which pylint hook to execute based on the plugin being modified so all pylint hooks are executed regardless of the specific plugin environment, which results in import errors.

@SajidAlamQB SajidAlamQB removed the request for review from noklam July 12, 2023 09:23
Makefile Outdated
@@ -14,7 +14,7 @@ install-pip-setuptools:
python -m pip install -U pip setuptools wheel

lint:
cd $(plugin) && pre-commit run -a --hook-stage manual
pre-commit run -a --hook-stage manual
Copy link
Member

Choose a reason for hiding this comment

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

Probably being naive again, but isn't the problem of conflicting dependencies that doing run -a (instead of only doing pylint-$(plugin) from that set)? The way things are currently set up.

Copy link
Member

Choose a reason for hiding this comment

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

More broadly, we have the general problem that monorepo support isn't great (existent) in pre-commit. Could consider a next step of trying ruff for just pylint (and then extending it more broadly)? I guess @noklam could have some thoughts based on his work on the main repo.

Copy link
Contributor Author

@SajidAlamQB SajidAlamQB Jul 17, 2023

Choose a reason for hiding this comment

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

Probably being naive again, but isn't the problem of conflicting dependencies that doing run -a (instead of only doing pylint-$(plugin) from that set)? The way things are currently set up.

I think this would be the best solution for now and we should look into ruff for kedro-plugins.

Instead of trying to run all hooks at once with, pre-commit run -a --hook-stage manual, in the check-plugin.yml i've broken it down into:

run: |
          pre-commit run trailing-whitespace --all-files
          pre-commit run end-of-file-fixer --all-files
          pre-commit run check-yaml --all-files
          pre-commit run check-added-large-files --all-files
          pre-commit run check-case-conflict --all-files
          pre-commit run check-merge-conflict --all-files
          pre-commit run debug-statements --all-files
          pre-commit run requirements-txt-fixer --all-files
          pre-commit run flake8 --all-files
          pre-commit run isort --all-files --hook-stage manual
          pre-commit run black --all-files --hook-stage manual
          pre-commit run secret_scan --all-files --hook-stage manual
          pre-commit run bandit --all-files --hook-stage manual
          pre-commit run pylint-${{inputs.plugin}} --all-files --hook-stage manual

Copy link
Contributor

Choose a reason for hiding this comment

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

ruff definitely helps for isort ,flake8 and pylint, many of these pre-commit libraries are pinned at an old version, so when I switch to ruff I get lots of error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated make lint it will now run each individual pre-commit hook instead of doing it in check-plugin.yml so we can continue to use make plugin=${{ inputs.plugin }} lintincheck-plugins.yml`.

The new make lint:

lint:
	pre-commit run trailing-whitespace --all-files && pre-commit run end-of-file-fixer --all-files && pre-commit run check-yaml --all-files && pre-commit run check-added-large-files --all-files && pre-commit run check-case-conflict --all-files && pre-commit run check-merge-conflict --all-files && pre-commit run debug-statements --all-files && pre-commit run requirements-txt-fixer --all-files && pre-commit run flake8 --all-files && pre-commit run isort-$(plugin) --all-files --hook-stage manual && pre-commit run black-$(plugin) --all-files --hook-stage manual && pre-commit run secret_scan --all-files --hook-stage manual && pre-commit run bandit --all-files --hook-stage manual && pre-commit run pylint-$(plugin) --all-files --hook-stage manual && pre-commit run pylint-$(plugin)-features --all-files --hook-stage manual && pre-commit run pylint-$(plugin)-tests --all-files --hook-stage manual

@SajidAlamQB SajidAlamQB requested review from noklam, deepyaman and merelcht and removed request for AhdraMeraliQB July 17, 2023 09:11
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

Thanks for exploring all the options with this setup, I agree that this is good for now and we can revisit when introducing ruff later on 👍

Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

Thanks for figuring it out, I guess with ruff it could be simplify more later.

Comment on lines 94 to 109
run: |
pre-commit run trailing-whitespace --all-files
pre-commit run end-of-file-fixer --all-files
pre-commit run check-yaml --all-files
pre-commit run check-added-large-files --all-files
pre-commit run check-case-conflict --all-files
pre-commit run check-merge-conflict --all-files
pre-commit run debug-statements --all-files
pre-commit run requirements-txt-fixer --all-files
pre-commit run flake8 --all-files
pre-commit run isort --all-files --hook-stage manual
pre-commit run black --all-files --hook-stage manual
pre-commit run secret_scan --all-files --hook-stage manual
pre-commit run bandit --all-files --hook-stage manual
pre-commit run pylint-${{inputs.plugin}} --all-files --hook-stage manual

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the make lint still useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update the make lint to run these instead

Copy link
Contributor Author

@SajidAlamQB SajidAlamQB Jul 19, 2023

Choose a reason for hiding this comment

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

Updated make lint it will now run each individual pre-commit hook with && the drawback from this is that it will stop running if one hook fails. The alternative is to use ; but CI won't detect if a hook fails so I've gone with &&. This also means we can go back to using make plugin=${{ inputs.plugin }} lint in check-plugins.yml.

lint:
	pre-commit run trailing-whitespace --all-files && pre-commit run end-of-file-fixer --all-files && pre-commit run check-yaml --all-files && pre-commit run check-added-large-files --all-files && pre-commit run check-case-conflict --all-files && pre-commit run check-merge-conflict --all-files && pre-commit run debug-statements --all-files && pre-commit run requirements-txt-fixer --all-files && pre-commit run flake8 --all-files && pre-commit run isort-$(plugin) --all-files --hook-stage manual && pre-commit run black-$(plugin) --all-files --hook-stage manual && pre-commit run secret_scan --all-files --hook-stage manual && pre-commit run bandit --all-files --hook-stage manual && pre-commit run pylint-$(plugin) --all-files --hook-stage manual && pre-commit run pylint-$(plugin)-features --all-files --hook-stage manual && pre-commit run pylint-$(plugin)-tests --all-files --hook-stage manual

Makefile Outdated
@@ -14,7 +14,7 @@ install-pip-setuptools:
python -m pip install -U pip setuptools wheel

lint:
cd $(plugin) && pre-commit run -a --hook-stage manual
pre-commit run -a --hook-stage manual
Copy link
Contributor

Choose a reason for hiding this comment

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

ruff definitely helps for isort ,flake8 and pylint, many of these pre-commit libraries are pinned at an old version, so when I switch to ruff I get lots of error.

@SajidAlamQB SajidAlamQB merged commit 31b311e into main Jul 19, 2023
@SajidAlamQB SajidAlamQB deleted the dev/pre-commit-config branch July 19, 2023 14:43
@SajidAlamQB SajidAlamQB mentioned this pull request Aug 24, 2023
4 tasks
PtrBld pushed a commit to PtrBld/kedro-plugins that referenced this pull request Aug 27, 2023
…o-org#259)

* Create .pre-commit-config.yaml

* Update .pre-commit-config.yaml

* remove pre-commit-config from each plugin

* fix relative paths

* add exclude regex

* Update .pre-commit-config.yaml

* rework pre-commit

* test

* Revert "test"

This reverts commit edf731e.

* Update Makefile

* Update .pre-commit-config.yaml

* Update .pre-commit-config.yaml

* Update .pre-commit-config.yaml

* Update .pre-commit-config.yaml

* TEST

* Update .pre-commit-config.yaml

* fix isort

* undo test

* lint

* add secret scan and bandit again

* pass_filenames: true

* Update check-plugin.yml

* Revert "pass_filenames: true"

This reverts commit 1e85806.

* disable import errors

* lint

* Update .pre-commit-config.yaml

* Update .pre-commit-config.yaml

* isort

* try running hooks seperatley

* revert telemetry isort

* fix ga yaml

* Update check-plugin.yml

* add hook stage

* changes based on review

* add features and tests for docker, airflow and telemetry

* Update Makefile

* indent

* lint

* lint for airflow and docker

* Update check-plugin.yml

* undo plugin lint
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.

5 participants