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

pre-requisite dependency(node or yarn) version check added in dev.sh file #589

Closed

Conversation

sahitya-chandra
Copy link
Contributor

@sahitya-chandra sahitya-chandra commented Oct 12, 2024

fixed #168

  1. First check
  • Node is installed or not
  • Compatible node version is installed or not
  1. Second check
  • Yarn is installed or not
  • Compatible yarn version is installed or not

If these two checks will pass then only ./dev.sh will run successfully

@jayantbh sir take a look

Copy link
Contributor

@jayantbh jayantbh left a 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() {
Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Contributor

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).

Copy link
Contributor

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.

@jayantbh
Copy link
Contributor

jayantbh commented Oct 15, 2024

REQUIRED_DOCKER_VERSION=22.3.1

How did you land on this version being required?
Not saying it's wrong, but I thought we need v24 at least. Unless you're sure that it works with v22 perfectly, you might want to change that to v24.

@sahitya-chandra
Copy link
Contributor Author

sahitya-chandra commented Oct 15, 2024

REQUIRED_DOCKER_VERSION=22.3.1

How did you land on this version being required? Not saying it's wrong, but I thought we need v24 at least. Unless you're sure that it works with v22 perfectly, you might want to change that to v24.

@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

@jayantbh
Copy link
Contributor

The earliest version that supports the docker watch and sync commands will be sufficient.

@jayantbh
Copy link
Contributor

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.
You fix that, then it says yarn version is wrong, and so on.

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?

@sahitya-chandra
Copy link
Contributor Author

@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

@sahitya-chandra
Copy link
Contributor Author

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. You fix that, then it says yarn version is wrong, and so on.

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?

on it sir

@sahitya-chandra
Copy link
Contributor Author

@jayantbh how my pr has 23 commits. I have not added this much

@jayantbh
Copy link
Contributor

You would need to rebase your branch onto main. This might have happened due to merging main into your branch.

@sahitya-chandra
Copy link
Contributor Author

@jayantbh Sir i have rebased it but it is still giving this pre-commit check error

@sahitya-chandra
Copy link
Contributor Author

sahitya-chandra commented Oct 15, 2024

Is it even possible to fix this error
should I close this pr it is always giving this ore-commit error

@sahitya-chandra sahitya-chandra deleted the pre-requisite-check branch October 16, 2024 14:30
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.

Can we please mention the version numbers for all pre-requisites to run this project?
2 participants