Merge lp:~jose/charms/precise/juju-gui/add-blank-defaults into lp:~juju-gui/charms/trusty/juju-gui/trunk

[José Antonio Rey]

Currently, `charm proof` returns an error when there are options without defaults on the config.yaml file. I've added blank defaults for the options that didn't have them, except for login-help.

[Matt Bruzek (mbruzek)]

Hello Jose,

Thank you for working on this merge proposal! Your commitment to the communit is very impressive.

The current charm store policy does recommend that all configuration values have default values. However some older charms got in before this was a rule and consiquently some code actually depends on the value not being set. Since the hooks are python that is realized as a value of None in the code. When you default it to "" the configuration option is not None. We should be very careful with these kind of changes, to not break the code.

I followed all 4 configuration values through the code and believe that 3 of them are OK. But the login-help configuration value has the following code:

if login_help is None:

So when the login-help configuration option is "" it misses this block of code and the hard coded text is never set. I was able to see this text if you deploy the current juju-gui charm on the login page. If you deploy the juju-gui with your code the login text is empty and I do not believe this is what we want to happen.

Please leave the login-help configuration value without a default. I realize that charm proof will still report a warning here but this is a rare case where that is OK.

Thanks again for your work here! Keep being awesome Jose!

review: Needs Fixing

[José Antonio Rey (jose)]

Hey, Matt!

Thanks so much for the comments, really appreciate it :)

I have removed the blank default from login-help. Let me know if there's anything else I should fix and I'll get right to it.

[Francesco Banconi (frankban)]

Hi José,

thanks a lot for contributing to the GUI charm!
FWIW I think it should be easy enough to change utils.write_gui_config so that it handles login_help as an empty string if you prefer that strategy.

That said, the GUI charm development is done using the development branch in lp:~juju-gui/charms/trusty/juju-gui/trunk (see http://bazaar.launchpad.net/~juju-gui-charmers/charms/precise/juju-gui/trunk/view/head:/HACKING.md). When we create a new release, that branch is then pushed to the precise and trusty release branches (lp:charms/juju-gui and lp:charms/trusty/juju-gui).

Could you please re-propose your changes against that branch?
Also, please run "make lint" and "make unittest" before creating the MP.

Thanks!

[José Antonio Rey (jose)]

Hi, Francesco!

I am going to make sure I do that later. Thanks for the heads up.

[Matt Bruzek (mbruzek)]

Hello José,

Thanks for addressing my concern about the login-help. I see no further blocking issues.

+1 LGTM.

My understanding is the juju-gui team will have to review and approve this change. Look for them to make comments on this change after they run their tests.

Thanks again for your patience on this process.

Unmerged revisions

90. By José Antonio Rey on 2014-04-24

Removed blank default from login-help

89. By José Antonio Rey on 2014-03-26

Added blank defaults for missing ones

=== modified file 'config.yaml'
--- config.yaml	2014-04-15 14:17:12 +0000
+++ config.yaml	2014-04-24 18:19:36 +0000
@@ -66,12 +66,14 @@
       the GUI. Both ssl-cert-contents and ssl-key-contents must be provided.
       If not, cetificates will be automatically generated.
     type: string
+    default: ""
   ssl-key-contents:
     description: |
       The contents of the private key file to be used in SSL connections to
       the GUI. Both ssl-cert-contents and ssl-key-contents must be provided.
       If not, cetificates will be automatically generated.
     type: string
+    default: ""
   login-help:
     description: |
       The help text shown to the user on the login screen. If not provided, a
@@ -103,6 +105,7 @@
       If given, the password to use for the environment to immediately
       connect.  Do not set unless you understand and accept the risks.
     type: string
+    default: ""
   sandbox:
     description: |
       Run using an in-memory sandbox rather than a real Juju backend. Sandbox