-
Notifications
You must be signed in to change notification settings - Fork 37
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
Feature: Add LogOperations to execution hooks #661
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## main #661 +/- ##
==========================================
+ Coverage 69.28% 69.55% +0.26%
==========================================
Files 44 45 +1
Lines 4295 4332 +37
Branches 1047 1051 +4
==========================================
+ Hits 2976 3013 +37
Misses 1108 1108
Partials 211 211
|
Names now match the names of the hook triggers. We also rename the install_hooks to specify that it installs hooks project wide.
The call method is confusing as it only applies to projects but hooks can be project or operation wide.
Allow for the correct passing of the project to install the hooks for.
Assumes that we are using different log files in LogOperation tests.
…when I tested the example.
@vyasr It's waiting on a review. @kidrahahjo If you have time, could you take a look at this PR again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a few minor suggestions.
flow/hooks/log_operations.py
Outdated
# Do something | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since many users typically just copy any example code verbatim, this is meant to emphasize that without a Project definition, nothing would actually happen. So an alternative would be to add at least one "hello world" operation to the example or make this comment a bit clearer, e.g., # Project operation definitions ..."
.
Co-authored-by: Bradley Dice <[email protected]>
Bumps [pre-commit](https://github.com/pre-commit/pre-commit) from 3.3.2 to 3.3.3. - [Release notes](https://github.com/pre-commit/pre-commit/releases) - [Changelog](https://github.com/pre-commit/pre-commit/blob/main/CHANGELOG.md) - [Commits](pre-commit/pre-commit@v3.3.2...v3.3.3) --- updated-dependencies: - dependency-name: pre-commit dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [ruamel-yaml](https://sourceforge.net/p/ruamel-yaml/code/ci/default/tree) from 0.17.31 to 0.17.32. --- updated-dependencies: - dependency-name: ruamel-yaml dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [pytest](https://github.com/pytest-dev/pytest) from 7.3.1 to 7.4.0. - [Release notes](https://github.com/pytest-dev/pytest/releases) - [Changelog](https://github.com/pytest-dev/pytest/blob/main/CHANGELOG.rst) - [Commits](pytest-dev/pytest@7.3.1...7.4.0) --- updated-dependencies: - dependency-name: pytest dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
updates: - [github.com/asottile/pyupgrade: v3.3.1 → v3.8.0](asottile/pyupgrade@v3.3.1...v3.8.0) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
f4190f9
to
6287212
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is very close. We should push this through before the next release.
2132486
to
672726b
Compare
Add builtin logging for execution hooks.
Description
Add
LogOperations
and tests forLogOperations
to execution hooks.Motivation and Context
Provides built in logging hooks for users. Partially addresses #637 and #508.
Checklist: