Skip to content

Conversation

@mohammad-alisafaee
Copy link
Contributor

Description

renku doctor command also checks for the existence of the git hooks and checks that their content is up-to-date.

Fixes #479

@mohammad-alisafaee mohammad-alisafaee requested a review from a team as a code owner December 10, 2019 14:11
Copy link
Member

@rokroskar rokroskar left a comment

Choose a reason for hiding this comment

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

Just a minor wording suggestion in the case of a missing hook template, feel free to ignore if you think it's not necessary to implement.

One thing that comes to mind though - should we run renku doctor automatically every once in a while? The downside is that the the shacl check can be slow. Or maybe we could just suggest to the user periodically to run renku doctor.

expected_hook = _extract_renku_hook(file_)

if not expected_hook:
message = WARNING + 'Cannot check for existence of Git hooks.\n'
Copy link
Member

Choose a reason for hiding this comment

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

I guess this would happen if the renku installation is somehow corrupted? Maybe we should suggest to reinstall renku?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was for myself to avoid comparing empty dicts in case the pattern was not parsed correctly :D. This is basically a safety-net for us in case we update the hooks and mess something up so that we see some tests fail. User should never see this and if something is wrong with their renku installation then they probably won't even reach this code.


if actual_hook != expected_hook:
message = WARNING + 'Git hooks are outdated or not installed. ' \
'Use "renku githooks install --force" to update them. \n'
Copy link
Member

Choose a reason for hiding this comment

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

This works nicely!

@mohammad-alisafaee mohammad-alisafaee merged commit 54ba91d into master Dec 12, 2019
@mohammad-alisafaee mohammad-alisafaee deleted the 479-check-pre-commit-hooks branch December 12, 2019 14:02
@mohammad-alisafaee
Copy link
Contributor Author

One thing that comes to mind though - should we run renku doctor automatically every once in a while? The downside is that the the shacl check can be slow. Or maybe we could just suggest to the user periodically to run renku doctor.

I am not normally a fan of doing automatic things without users knowing about them. We probably can do this (or ask users to do it) only when there is an update in renku version to make sure that project is compatible with a newer version. I'll create a story for that.

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.

cli: renku doctor should check pre-commit hooks

3 participants