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

Add pre-commit and fix findings #601

Merged
merged 11 commits into from Jan 12, 2023
Merged

Add pre-commit and fix findings #601

merged 11 commits into from Jan 12, 2023

Conversation

dbast
Copy link
Member

@dbast dbast commented Jan 2, 2023

Description

Inspired by the .pre-commit-config.yaml in conda/conda-build/conda-pack where similar PRs have been done in the past.

The .pre-commit-config.yaml has been stripped down to mainly only contain the flake8 and shellcheck hooks for keeping the number of changed lines as small as possible. More hooks (isort, pyupgrade) can be incrementally enabled via more PRs. Similar PRs have been e.g. merged to conda without causing too much trouble with existing open PRs.

Getting some linting running automatically is of great help when reviewing PRs. This once requires all current finding being fixed, which is done as part of this PR.

This also finds some not existing code in conda_interface.py where the function mkdir_p_sudo_safe(cache_dir) does not exist.

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Jan 2, 2023
@jaimergp
Copy link
Contributor

I am aching to do this with the codebase but it will create conflicts on opened PRs so I am inclined to merge as much as possible before we merge this one to minimize the amount of rebasing hell we need :D

@dbast
Copy link
Member Author

dbast commented Jan 11, 2023

@jaimergp going to strip down the pre-commit config to reduce the amount of changes... more pre-commit hooks in be incrementally added via more PRs.

@dbast dbast marked this pull request as ready for review January 11, 2023 13:16
@dbast dbast requested a review from a team as a code owner January 11, 2023 13:16
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@jaimergp
Copy link
Contributor

Ok, that makes more sense. Maybe using blacken in the next PR is a good step too.

@jezdez
Copy link
Member

jezdez commented Jan 12, 2023

@dbast I've just added the precommit.ci app to this repo. https://results.pre-commit.ci/repo/github/51571636

Badge: pre-commit.ci status

[![pre-commit.ci status](https://results.pre-commit.ci/badge/github/conda/constructor/main.svg)](https://results.pre-commit.ci/latest/github/conda/constructor/main)

.github/workflows/main.yml Outdated Show resolved Hide resolved
Co-authored-by: Jannis Leidel <jannis@leidel.info>
@dbast
Copy link
Member Author

dbast commented Jan 12, 2023

@jezdez Any idea how to extend the pre-commit,ci to be able to execute hooks that need additional tools? like the shellcheck hook which requires Docker (or in other cases hooks that require golang)? That is pretty easy with the pre-commit action, as there are anyways more tools pre-installed on the action runners and also extending those can be done via an action step running before the pre-commit step.

This repo has many shellscripts as part of the code and as part of the examples and as shellcheck pretty good detects all kind of issues like unquoted variables, posix compatibility, missing error handling etc.

This PR only checks the example shell scripts via shellcheck. The shell scripts in code are templated and require rendering before linting them via shell check; that is done in this PR #600

The pre-commit ci action is also also pretty fast as it also uses caching of the pre-commit hooks specific to this repo. One run = 23 secs (including shellcheck).

@dbast
Copy link
Member Author

dbast commented Jan 12, 2023

@jezdez we can also invite the (public) renovate bot which to keeps the .pre-commit-config.yml hooks versions updated via PRs it proposes... renovate can also update arbitrary pinning versions, if the right regex pattern are added. That would e.g. help to pin the micromamba version in #605 to have stable CI and have it updated by renovate.

@jaimergp
Copy link
Contributor

What about this alternative? https://pypi.org/project/shellcheck-py/

@dbast
Copy link
Member Author

dbast commented Jan 12, 2023

@jaimergp hah. good finding. trying that.

@dbast dbast merged commit cb0c37b into conda:main Jan 12, 2023
@dbast dbast deleted the pre-commit branch January 12, 2023 16:44
@github-actions github-actions bot added the locked [bot] locked due to inactivity label Jan 13, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed [bot] added once the contributor has signed the CLA locked [bot] locked due to inactivity
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants