Code review comment for lp:~hatch/juju-gui/save-changed-1214087

Revision history for this message
Jeff Pihach (hatch) wrote :

Thanks for the review - comments below and changes incoming.

I agree with your comments but where noted we can discuss in more detail
tomorrow.

https://codereview.appspot.com/13252045/diff/11001/app/store/env/go.js
File app/store/env/go.js (right):

https://codereview.appspot.com/13252045/diff/11001/app/store/env/go.js#newcode130
app/store/env/go.js:130: genericConstraints: ['cpu-power', 'cpu-cores',
'mem', 'arch'],
I didn't want to change everywhere else in the application that accessed
the constraints objects from either back end and all of their associated
tests.

https://codereview.appspot.com/13252045/diff/11001/app/views/ghost-inspector.js
File app/views/ghost-inspector.js (right):

https://codereview.appspot.com/13252045/diff/11001/app/views/ghost-inspector.js#newcode130
app/views/ghost-inspector.js:130: // PyJuju does not rquire some
constraints to be
On 2013/09/02 21:19:28, rharding wrote:
> r*e*require

Done.

https://codereview.appspot.com/13252045/diff/11001/app/views/ghost-inspector.js#newcode132
app/views/ghost-inspector.js:132: var integerConstraints =
this.options.env.integerConstraints;
That's a good idea, done.

https://codereview.appspot.com/13252045/diff/11001/app/views/inspector.js
File app/views/inspector.js (right):

https://codereview.appspot.com/13252045/diff/11001/app/views/inspector.js#newcode741
app/views/inspector.js:741: env.set_config(
Lets discuss this tomorrow - I figured that this method may be called
directly and not have it's values run through this method.

https://codereview.appspot.com/13252045/diff/11001/app/views/utils.js
File app/views/utils.js (right):

https://codereview.appspot.com/13252045/diff/11001/app/views/utils.js#newcode670
app/views/utils.js:670: utils.removeUnchangedConfigOptions =
function(config, charmOptions) {
As previously mentioned - lets discuss tomorrow.

https://codereview.appspot.com/13252045/diff/11001/test/test_env_python.js
File test/test_env_python.js (right):

https://codereview.appspot.com/13252045/diff/11001/test/test_env_python.js#newcode74
test/test_env_python.js:74: var constraints = { cpu: '1', mem: '512M',
arch: 'i386'};
I don't think so, probably just an old habit.

https://codereview.appspot.com/13252045/

« Back to merge proposal