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

feat(core): fix autocommit LFS files in pre-commit hook #2245

Merged
merged 4 commits into from Aug 11, 2021

Conversation

m-alisafaee
Copy link
Contributor

Description

Fixes handling of arrays in the pre-commit hook. Users need to do renku githooks uninstall and renku githooks install in the existing repos after updating the renku version.

Fixes #2230

@m-alisafaee m-alisafaee force-pushed the 2230-lfs-autocommit-not-working branch from b94faa9 to ecd147a Compare August 6, 2021 17:46
@m-alisafaee m-alisafaee force-pushed the 2230-lfs-autocommit-not-working branch from ecd147a to 5cf2ac7 Compare August 6, 2021 20:06
@m-alisafaee m-alisafaee marked this pull request as ready for review August 9, 2021 07:16
@m-alisafaee m-alisafaee requested a review from a team as a code owner August 9, 2021 07:16
@Panaetius
Copy link
Member

Since users need to take a manual action to fix this, we should have some way to inform them of this.

I seems pip itself doesn't support anything like that.
And implementing some mechanism on our end to show a warning the first time a command is run after a user has upgrade renku is probably too much effort. Though it might be useful to have in general, not just for this?
But if we just write it in the changelog, I'm sure it'll get lost for most users

@m-alisafaee
Copy link
Contributor Author

Since users need to take a manual action to fix this, we should have some way to inform them of this.

I seems pip itself doesn't support anything like that.
And implementing some mechanism on our end to show a warning the first time a command is run after a user has upgrade renku is probably too much effort. Though it might be useful to have in general, not just for this?
But if we just write it in the changelog, I'm sure it'll get lost for most users

We have renku doctor check for githooks which warns users if their project's githooks is outdated. Maybe we can somehow include the githooks in the migrations. That would required a proper design.

@Panaetius
Copy link
Member

Yes it's probably best done in a followup issue.

Something to let users know of important changes after an update might be nice (not just for hooks).

@m-alisafaee m-alisafaee merged commit 78fad89 into master Aug 11, 2021
@m-alisafaee m-alisafaee deleted the 2230-lfs-autocommit-not-working branch August 11, 2021 13:16
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.

bug: Git LFS autocommit fails
2 participants