-
Notifications
You must be signed in to change notification settings - Fork 119
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
pre-requisite dependency(node or yarn) version check added in dev.sh file #589
pre-requisite dependency(node or yarn) version check added in dev.sh file #589
Conversation
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.
This file should also check for the minimum docker versions, since dev.sh requires 3 things to run:
Node, Yarn, and Docker.
Everything else is within the containers.
dev.sh
Outdated
REQUIRED_NODE_VERSION=22.9.0 | ||
REQUIRED_YARN_VERSION=1.22.22 | ||
|
||
version_ge() { |
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.
Was this supposed to be version_get
?
Or maybe you meant greater-than or equal-to.
IMO naming the function compare_version
or version_at_least
or something more obvious would be better.
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 have now changed it to version_at_least
dev.sh
Outdated
@@ -1,5 +1,46 @@ | |||
#!/bin/bash | |||
|
|||
REQUIRED_NODE_VERSION=22.9.0 |
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 we're not strict on Node 22.9. It can be anything above 22 (for now).
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.
Yarn does need to be 1.22.22 at least.
How did you land on this version being required? |
@jayantbh Sorry sir, I want it to 27.3.1 and this is because I have read docker docs it is the latest docker version and I also use this version locally should I change it to 27.0.0 or 26 or something |
The earliest version that supports the docker watch and sync commands will be sufficient. |
I have an idea that'll make the version check experience better. Right now, let's say all your versions are wrong... Then you run dev.sh, and it breaks saying the node version is wrong. What if we check all breaking versions without killing the script, and then list breaking versions of everything at the end right before killing the script? |
@jayantbh Sir, I have changed the docker version 24.0.0 because it is docker watch was added and can you tell me why my pre-commit checks are failing |
on it sir |
@jayantbh how my pr has 23 commits. I have not added this much |
You would need to rebase your branch onto main. This might have happened due to merging main into your branch. |
@jayantbh Sir i have rebased it but it is still giving this pre-commit check error |
Is it even possible to fix this error |
fixed #168
If these two checks will pass then only ./dev.sh will run successfully
@jayantbh sir take a look