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

Update precommit #1080

Merged
merged 2 commits into from
Dec 14, 2022
Merged

Update precommit #1080

merged 2 commits into from
Dec 14, 2022

Conversation

choldgraf
Copy link
Collaborator

@choldgraf choldgraf commented Dec 14, 2022

On systems that don't have the right version of GLIBC installed pre-commit will not work because of a missing dependency as described in this SO post:

This fixes the version of node that is used so that this issue doesn't pop up, as per the guidelines in that issue.

While I did this, I also updated the pre-commit versions with pre-commut autoupdate and ran pre-commit run -a

@12rambau
Copy link
Collaborator

if this fix is specifcally designed for bitbucket would it be possible to test it in the test suit ?

@choldgraf
Copy link
Collaborator Author

mmmm no it is not designed for bitbucket, but I ran into this bug on my machine and this fixed it, and I didn't see any downside in fixing the node version for pre-commit so I went with what the SO post recommended

@12rambau
Copy link
Collaborator

Legitimate, I though it was for a very specific and narrow application. Nothing to complain about from my side, as long as it works everywhere I'm happy !

@drammock
Copy link
Collaborator

this seems probably fine... but that SO answer says (option 3)

install node into your image

this will skip the "download node" step of pre-commit by default (specifically it'll default to language_version: system when a suitable node is detected).

Don't we require node anyway? Why is precommit trying to install it again?

@choldgraf
Copy link
Collaborator Author

Why is precommit trying to install it again?

I don't know - honestly I didn't have a lot of time to investigate the "why" behind all of this, but my build for this repo was broken and this fixed it + seemed harmless since it's just for pre-commit.

@12rambau
Copy link
Collaborator

then here we go !

@12rambau 12rambau merged commit 862acfe into pydata:main Dec 14, 2022
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.

3 participants