-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Improved getting started #3128
Improved getting started #3128
Conversation
Sql.js is not needed when we have SQLite, and oracle/mssql have never been supported by Vendure
Since v2.2, Vendure no longer supports Yarn v1.x which is long unmaintained, and breaks with some of the depdendencies we use. At the same time, modern Yarn (berry) does not get installed globally - rather it uses corepack which is specific to a particular package dir. Therefore all the logic around detecting a global Yarn binary (which was all inherited from the Create React App code) is no longer relevant.
Prevents the progress bar from interfering with the vendure create clack prompt spinner
Relates to #3117
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…docker compose file
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.
Ship it 🚀
function isDaemonRunning(): boolean { | ||
try { | ||
execFileSync('docker', ['stats', '--no-stream'], { stdio: 'ignore' }); | ||
return true; | ||
} catch (e: any) { | ||
return false; | ||
} | ||
} |
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.
Some setups will require root privileges for the docker stats
command so this might fail even if technically the daemon is running.
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.
That's true, but we won't be able to cover 100% of all setups.
packages/create/src/helpers.ts
Outdated
} else { | ||
execSync('systemctl start docker'); | ||
} |
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.
Might be worth to enable
instead of start
to make docker auto-start on next reboot. Sidenote: Both starting and enabling require root privileges which might trip up some OS'
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 command is running only on Linux based OS. In most cases the primary user has administrator rights. So again, covering 100% of setups (especially Linux-based) is virtually impossible. I think we can iterate here based on user feedback
Recommended by the code scan as a better way that is not as vulnerable to injection
Thanks for your input @DanielBiegler! As David mentioned, there will certainly be cases where the automated Docker control will fail - in this case we have the flow "we couldn't start Docker" and then offer the user the chance to start it manually, or opt to setup with SQLite instead. |
Relates to #3117