-
Notifications
You must be signed in to change notification settings - Fork 997
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(baremetal): Check available disk space #11469
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.
Interesting...I probably would have just added this as a lifecycle thing (adding a [before]
hook in deploy.toml
) but I can see this being useful for others.
Instead of hardcoding the size to 2GB let's put that value in deploy.toml
and let you customize it for your app—set it to something like freeSpaceRequired=2048
in newly generated deploy.toml
files, and maybe if the value doesn't exist at all use 2048
by default so that it's backwards compatible for those that don't add the option.
How sure are you about that awk
command working for different flavors of *nix df
output? Maybe on lines 389 and 395 of baremetal.js
you just output a warning if you can't read the output, instead of throwing an error? Because if it can't be read, that's our problem for not parsing the output of df
correctly, not the user's. They'll be confused thinking they did something wrong. If we only throw errors then they'll need to add --no-df
flag every time they deploy, forever (we shouldn't ever make people do that).
To avoid setting that flag we could do something like allow them to set freeSpaceRequired=false
and it'll always skip, or freeSpaceRequired=0
if you have moral objections to two data types in a single variable 😉 (but obviously it should skip all the df
checks altogether, not still do them and just check if the result is > 0
or we'd back in the same boat still not being able to parse the output).
Also, need docs updates!
@cannikin Thanks for your review. Great suggestions as usual 🙂 I think I implemented them all (except the docs updates). Please have another look and let me know what you think. I also added some screenshots to the PR description you can look at.
|
@@ -173,6 +174,7 @@ This lists a single server, in the `production` environment, providing the hostn | |||
- `repo` - The path to the git repo to clone | |||
- `branch` - [optional] The branch to deploy (defaults to `main`) | |||
- `keepReleases` - [optional] The number of previous releases to keep on the server, including the one currently being served (defaults to 5) | |||
- `freeSpaceRequired` - [optional] The amount of free space required on the server in MB (defaults to 2048 MB). You can set this to `0` to skip checking. |
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 says [optional]
but doesn't the task itself require it to exist? It only skips if the value is set to 0
but what if you don't have the config option at all? Like if a person upgrades and doesn't this config var, what happens? Can it just also skip if undefined
?
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.
doesn't the task itself require it to exist?
No, I made it part of DEFAULT_SERVER_CONFIG
. So if it's not set at all it will default to 2048 MB.
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.
The deploy.toml
template should include the new freeSpaceRequired
var set at bottom probably?
Done ✅ |
Adds an early step to the
yarn rw deploy baremetal
command that checks to make sure there is enough free disk space on the server before continuing.I ran into an issue over the weekend where I was getting the weirdest errors when trying to deploy. It would just fail at random steps during the deploy process. Clearing up some space on the server fixed it. I had a little bit over 1.5GB free when it happened. A typical deploy seems to take up about 720MB for me right now. So it seems I needed more than twice that size for a successful deploy.
For now I decided to check for at least 2 GB free space. Less than that and the deploy will show a warning or error out with a more helpful error message than the errors I was getting 😅
If the user wants to bypass the check they can just pass
--no-df
or configurefreeSpaceRequired
to be 0.Example output:
The last one there is what it looks like when you have
freeSpaceRequired
set to0
(Please ignore the red
>
on the ones that are supposed to be passing, it's just because I'm throwing an error later during testing to not actually deploy anything)Existing projects deploying to servers with very little free disk space available will see a warning like this if they leave the
freeSpaceRequired
setting not configured