Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Implement Model.add_machine() #56
Conversation
|
Should we also change |
| - placement = [ | ||
| - client.Placement(**p) for p in to | ||
| - ] | ||
| + placement = parse_placement(to) |
petevg
Feb 21, 2017
Collaborator
This makes a lot of sense. Thank you.
You can technically get rid of the call to parse_placement in application.py (though the "backwards compatibility" line in placement.py means that the multiple calls won't break anything).
tvansteenburgh
Feb 22, 2017
Member
You mean the one in Application.add_unit()? AFAICT that's still needed, what am I missing?
| +from juju.model import Model | ||
| + | ||
| +MB = 1024 * 1024 | ||
| +GB = MB * 1024 |
petevg
Feb 21, 2017
Collaborator
If you take a look in constraints.py, these conversions are already built, and they use megabytes, rather than bytes, as a unit, per the Go code on the core side. I'm fairly certain that these numbers will give you machines with much more memory than expected (unless I'm missing a line where you downsize stuff before passing it to the API.)
| return [ | ||
| - client.Placement(scope=MACHINE_SCOPE, directive=machine), | ||
| - client.Placement(scope=container, directive=container_num) | ||
| + client.Placement(scope=MACHINE_SCOPE, directive=directive), | ||
| ] |
petevg
requested changes
Feb 21, 2017
Overall, I am +1 on this, but I think that you want to use Megabytes as your units for RAM, rather than bytes. (Feel free to ignore and merge anyway if I'm wrong.)
sparkiegeek
commented
Feb 22, 2017
|
Unit tests |
|
@johnsca Re: BundleHandler, I thought about it, but they are different enough that we'd need to factor some stuff out into a 3rd (private) method anyway. Since these are the only two callers, I don't think it's worth it. Maybe later if we end up with more code using the AddMachines api. |
|
@sparkiegeek Yep, coming. I knew you'd be watching. ;) Thanks for keeping me honest. |
tvansteenburgh
added some commits
Feb 23, 2017
tvansteenburgh commentedFeb 21, 2017
Also fixes a bug that was causing the 'to' parameter to
Model.deploy() to not be handled correctly.
Add docs and examples for adding machines and containers
and deploying charms to them.
Fixes #51.