console: validate disk size earlier, update error wording #596
Conversation
|
Note to reviewers: I've put "Fixes: harvester/harvester#4734" in the commit message, which will presumably automatically close that issue if this PR is merged. I'm not sure if that's usually done with harvester PRs. If not, please let me know and I'll remove that from the commit message. |
|
@tserong Thanks for the great PR! No, we don't like GitHub automatically closing an issue unless QA verifies it. So prefer not to use the fix/close keywords in the commits or PR. |
|
Thanks @bk201, I'll fix the commit message. I've also just done some more testing and have discovered that by setting |
tserong
force-pushed
the
wip-clarify-disk-size-errors
branch
from
5e4963c to
ebc9017
Compare
|
OK, that should be better now. |
|
Hi @tserong, please force push again to trigger CI. |
This commit adds disk size validation in the calls to diskConfirm() and dataDiskConfirm(), so that as you walk through the fields you'll see potential error messages as you go, rather than having to wait until leaving the last field on the page. The error message clears if you press the up arrow to go to previous fields. The persistent partition size check remains where it is, i.e. it's only done when you try to move to the next page. I couldn't figure out a neat way to incorporate this into the validateAllDiskSizes() function, which gets called before the default persistent size is set in persistentSizeV.PreShow(). This commit also changes the wording of the "disk size is too small" error messages to explicitly state *which* disk is too small, and makes a small tweak to the "partition size" error message. Signed-off-by: Tim Serong <[email protected]>
tserong
force-pushed
the
wip-clarify-disk-size-errors
branch
from
ebc9017 to
1f08ce6
Compare
|
Thanks @Vicente-Cheng that seems to have worked :-) |
| @@ -17,12 +17,12 @@ const ( | |||
|
|
|||
| func ParsePartitionSize(diskSizeBytes uint64, partitionSize string) (uint64, error) { | |||
| if diskSizeBytes < MinDiskSize { | |||
| return 0, fmt.Errorf("Disk size is too small. Minimum %dGi is required", ByteToGi(MinDiskSize)) | |||
| return 0, fmt.Errorf("Installation disk size is too small. Minimum %dGi is required", ByteToGi(MinDiskSize)) | |||
There was a problem hiding this comment.
nit: this makes the function only useful for the installation disk. Although we don't use the function in the data disk path, can we make the function more general like supporting passing a disk type?
The other parts of the PR work great!
There was a problem hiding this comment.
Although we don't use the function in the data disk path, can we make the function more general like supporting passing a disk type?
I'm not sure. I mean, yeah, we could add a parameter to specify disk type :-) but looking at the rest of the function, it's specifically testing sizes against MinDiskSize and MinPersistentSize, so ISTM the way it's implemented now is really geared towards looking at the installation disk only. Maybe this is something we can look at refactoring separately?
|
@Mergifyio backport v1.2 |
✅ Backports have been created
|
Problem:
The error messages are sometimes unclear when one has an installation disk that's too small.
Solution:
This commit adds disk size validation in the calls to
diskConfirm()anddataDiskConfirm(), so that as you walk through the fields you'll see potential error messages as you go, rather than having to wait until leaving the last field on the page. The error message clears if you press the up arrow to go to previous fields.The persistent partition size check remains where it is, i.e. it's only done when you try to move to the next page. I couldn't figure out a neat way to incorporate this into the
validateAllDiskSizes()function, which gets called before the default persistent size is set inpersistentSizeV.PreShow().This commit also changes the wording of the "disk size is too small" error messages to explicitly state which disk is too small, and makes a small tweak to the "partition size" error message.
Related Issue:
harvester/harvester#4734
Test plan: