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

add the minimum resource error improvement #2950

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

georgeliao
Copy link
Contributor

@georgeliao georgeliao commented Mar 6, 2023

fixes #2925
This is still a draft implementation, be free to give your comments on it.
The current output error message is like
./multipass launch docker --name load-test --cpus 1 --memory 2G --disk 20GiB

launch failed: Requested Number of CPUs is less than Blueprint minimum of 2
The docker requires at least 2 CPUs, 4G of memory and 40G of disk space.

@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Merging #2950 (683b5a3) into main (6ef9ce3) will increase coverage by 0.01%.
The diff coverage is 96.66%.

@@            Coverage Diff             @@
##             main    #2950      +/-   ##
==========================================
+ Coverage   86.89%   86.90%   +0.01%     
==========================================
  Files         231      231              
  Lines       12042    12061      +19     
==========================================
+ Hits        10464    10482      +18     
- Misses       1578     1579       +1     
Impacted Files Coverage Δ
...nclude/multipass/exceptions/blueprint_exceptions.h 95.00% <95.00%> (-5.00%) ⬇️
...ueprint_provider/default_vm_blueprint_provider.cpp 99.22% <100.00%> (+<0.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@georgeliao georgeliao marked this pull request as draft March 6, 2023 20:20
Copy link
Collaborator

@townsend2010 townsend2010 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @georgeliao!

Thanks for this!

I kind of feel that instead of making a call to prepopulate an error message that may or not happen, it may be better to modify BlueprintMinimumException to accept the limits YAML::Node instead and let it be responsible for constructing the error message when needed. I hope this makes sense 🙂

@georgeliao
Copy link
Contributor Author

@townsend2010
Thanks for your comment. I like what you said "may or not happen". It is something I overlooked. Indeed, the error message generation should happen inside the if and throw blocks. I will refine the code.

@georgeliao
Copy link
Contributor Author

Hi @townsend2010

The comment is addressed. Please also feel free to comment further if there are still things to improve.

The current output of the program is:
./multipass launch docker --name load-test --cpus 1 --memory 2G --disk 10GiB

launch failed: Requested Number of CPUs is less than Blueprint minimum.
The docker requires at least 2 CPUs, 4G of memory and 40G of disk space.

@georgeliao georgeliao marked this pull request as ready for review March 20, 2023 22:05
@ricab ricab removed their request for review June 20, 2023 10:26
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.

Consolidate minimum resource errors
2 participants