Code review comment for lp:~wallyworld/juju-core/fix-instance-type-matching

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

LGTM with some suggestions.

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instancetype.go
File environs/instances/instancetype.go (right):

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instancetype.go#newcode98
environs/instances/instancetype.go:98: // (previously an instance type
with largest available memory was returned even if it didn't
I don't think we need to explain how it was previously.

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instancetype.go#newcode105
environs/instances/instancetype.go:105: minMemCons := ic.Constraints
Why not just:
cons.Mem = uint64(minMemoryHeuristic)
?

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instancetype_test.go
File environs/instances/instancetype_test.go (right):

https://codereview.appspot.com/58950043/diff/40001/environs/instances/instancetype_test.go#newcode155
environs/instances/instancetype_test.go:155: about: "small mem
specified, use that even though less than needed for mongodb",
I think if the user overrode our minimum memory heuristic, we should at
least print out a warning, saying bootstrap might fail due to
insufficient memory for mongo.

https://codereview.appspot.com/58950043/

« Back to merge proposal