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.

Sign up for GitHub

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

Jump to bottom

[Pre Commit] infrastructure enhancements #3850

Merged
merged 122 commits into from
Dec 14, 2023
Merged

[Pre Commit] infrastructure enhancements #3850

merged 122 commits into from
Dec 14, 2023

Conversation

ilaner
Copy link
Contributor

@ilaner ilaner commented Dec 7, 2023 •
edited
Loading

This adds the following features:

  1. Support env property inside the docker hook, to provide extra env variables to the hook.
  2. support copy_files inside the docker hook, to support copy files to the integration script folders dynamically (the conftest.py has to be in the same directory of running tests)
  3. Get the test image without pulling if it exists, and pull in the background (if it's not a dry run)
  4. Filters ruff files by py files only, significantly speeds up execution.
  5. Run validate -a if pre-commit -a, because the CLI can't manage so many inputs to validate and it crashes.
  6. Delete all run-unit-tests command.
  7. Add support for running specific hooks.
  8. Run docker hooks in multiprocessing.
  9. Skipped hooks will not be generated in the config file.
  10. Force colorful output
  11. Add support for system hooks. system hooks will run in the same interpreter that pre-commit runs and not in virtual environment (useful for demisto-sdk commands)
  12. Moved the command to typer.

fixes: CIAC-8862
fixes: https://jira-dc.paloaltonetworks.com/browse/CIAC-8987

needs content small PR:
related: CIAC-7992
related: CIAC-7993

related conent PR: demisto/content#31224

@ilaner
Copy link
Contributor Author

ilaner commented Dec 12, 2023

@JudahSchwartz
PR is ready for rereview.
linked relevant issues.

JudahSchwartz
JudahSchwartz approved these changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@JudahSchwartz JudahSchwartz left a comment

Choose a reason for hiding this comment

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

Amazing work!!!!

demisto_sdk/commands/pre_commit/README.md Outdated
```
This key could be use together with [mode](#modes), to skip certain hook when running in a specific mode.

# Needs key
Copy link
Contributor

Choose a reason for hiding this comment

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

how are you deciding the header level here 🤨

demisto_sdk/commands/pre_commit/README.md
This key could be use together with [mode](#modes), to skip certain hook when running in a specific mode.

# Needs key
Needs keys allow to define dependencies between hooks. If a hook with `needs` is skipped, hooks that depend on it will also be skipped.
Copy link
Contributor

Choose a reason for hiding this comment

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

needs doesnt guarantee order? are you sure we dont want to just make them use skip:true for this? Seems like we're complicating things

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops

JudahSchwartz
JudahSchwartz reviewed Dec 13, 2023
View reviewed changes
demisto_sdk/commands/pre_commit/pre_commit_command.py
and any("in-docker" in need for need in needs)
}

def _get_docker_and_no_docker_hooks(
Copy link
Contributor

Choose a reason for hiding this comment

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

docstrings to all these functions please

@coveralls
Copy link
Collaborator

coveralls commented Dec 14, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 5e99e846-919a-4aa8-b1b3-fb0d4d537c13

  • 187 of 266 (70.3%) changed or added relevant lines in 11 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.02%) to 81.05%
Changes Missing Coverage Covered Lines Changed/Added Lines %
demisto_sdk/commands/pre_commit/hooks/hook.py 9 12 75.0%
demisto_sdk/commands/common/docker_helper.py 10 31 32.26%
demisto_sdk/commands/pre_commit/hooks/docker.py 36 59 61.02%
demisto_sdk/commands/pre_commit/pre_commit_command.py 115 147 78.23%
Files with Coverage Reduction New Missed Lines %
demisto_sdk/commands/pre_commit/pre_commit_command.py 1 84.06%
Totals Coverage Status
Change from base Build 8c963d46-f211-460c-bda3-eba02b145ed7: -0.02%
Covered Lines: 34007
Relevant Lines: 41958
💛 - Coveralls

@ilaner ilaner merged commit 0b17afe into master Dec 14, 2023
18 checks passed
@ilaner ilaner deleted the dont_pull_images_in_docker_hook branch December 14, 2023 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JudahSchwartz JudahSchwartz JudahSchwartz approved these changes

@GuyAfik GuyAfik GuyAfik left review comments

@dantavori dantavori Awaiting requested review from dantavori dantavori is a code owner

Assignees

@tschanfeld tschanfeld

Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@ilaner @JudahSchwartz @coveralls @GuyAfik @tschanfeld