Disable containerization when not supported. #531
Conversation
|
|
||
| @class MachineViewPanelNoopHeaderView | ||
| */ | ||
| var MachineViewPanelNoopHeaderView = Y.Base.create( |
There was a problem hiding this comment.
what's the advantage of a noop header vs adding a 'disabled' state to the current header?
There was a problem hiding this comment.
I think that having this separate view complicates things more than is necessary. You should be able to pass a flag into the header view which, on instantiation, executes the 'noop' init and render cycle. Then the attribute which is set can change, which would then run the 'setup' for the real header layout and enable the 'full' render method.
There was a problem hiding this comment.
A disabled state is a state, and it complicates stuff (adding conditions to all the methods, adding a test for each of those cases, more possibility to future regressions).
Toggling components seemed to me less error prone and easier to understand. The real view can do its job, while the "dummy" one is very simple.
There was a problem hiding this comment.
Thanks for the discussion on IRC. I didn't at first see the test that makes sure the dummy and real classes implement the same public interface.
|
Test PASSed. |
| // Define features exposed by each Juju provider type. | ||
| // To enable/disable containerization in the machine view, just add/remove | ||
| // supportedContainerTypes to the provider types below. | ||
| environments.providerFeatures = { |
There was a problem hiding this comment.
I wonder if these shouldn't be defined in an external configuration file?
There was a problem hiding this comment.
Well, provider stuff is something related to environments, but I am open to suggestions.
There was a problem hiding this comment.
I don't feel strongly about it. They just might be easier to find/modify later if they are in an external file.
I'll leave it up to you :)
|
My apologies for the long diff, but the task was not QA: In sandbox mode.
In a real environment (e.g. local LXC or EC2).
Done, thank you. |
|
Thanks for the discussion this looks really awesome. Note: Yes the icons not showing is a bug: https://bugs.launchpad.net/juju-gui/+bug/1364622 |
|
👍 thanks for all the additional clean up, that really clarifies things. No QA yet. |
|
Thank you all for the reviews! |
|
Status: merge request accepted. Url: http://ci.jujugui.org:8080/job/juju-gui-merge |
Disable containerization when not supported. Implement an infrastructure to dynamically disable containers support in the machine view based on the environment's provider type. A list of supported containers is defined at environment level, and used to check containerization support. Avoid hard-coding the list of container types in the create machine view. Update the service unit token to accept a list of available container types. Also removed the env from the service usint token: it doesn't appear to be used at all. This branch also includes some drive-by fixes to methods' docstrings and comments.
Implement an infrastructure to dynamically disable
containers support in the machine view based on the
environment's provider type.
A list of supported containers is defined at
environment level, and used to check containerization
support.
Avoid hard-coding the list of container types in the
create machine view.
Update the service unit token to accept a list of
available container types.
Also removed the env from the service usint token:
it doesn't appear to be used at all.
This branch also includes some drive-by fixes to
methods' docstrings and comments.