wordpress-charmers team mailing list archive

[Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx>]

Review: Approve

Yup.

Consider dropping the type annotations or deciding to add more of them. My code has them because I'm experimenting, and haven't got a firm recommendation one way or the other.

Diff comments:

> diff --git a/src/charm.py b/src/charm.py
> index a4cd679..e6af8ed 100755
> --- a/src/charm.py
> +++ b/src/charm.py
> @@ -61,6 +63,29 @@ def generate_pod_config(config, secured=True):
>      return pod_config
>  
>  
> +def _leader_get(attribute: str):

If you think the type annotations are a good step forward, great. If not, drop them in _leader_set and _leader_get and the import of typing on line 12. Don't just keep them by default, as you are sending mixed messages to future travelers.

> +    cmd = ['leader-get', '--format=yaml', attribute]
> +    return safe_load(subprocess.check_output(cmd).decode('UTF-8'))
> +
> +
> +def _leader_set(settings: Dict[str, str]):
> +    cmd = ['leader-set'] + ['{}={}'.format(k, v or '') for k, v in settings.items()]
> +    subprocess.check_call(cmd)
> +
> +
> +def create_wordpress_secrets():
> +    for secret in WORDPRESS_SECRETS:
> +        if not _leader_get(secret):
> +            _leader_set({secret: password_generator(64)})
> +
> +
> +def gather_wordpress_secrets():
> +    rv = {}
> +    for secret in WORDPRESS_SECRETS:
> +        rv[secret] = _leader_get(secret)
> +    return rv
> +
> +
>  class WordpressInitialiseEvent(EventBase):
>      """Custom event for signalling Wordpress initialisation.
>  
> diff --git a/src/wordpress.py b/src/wordpress.py
> index 6da3e69..a3f586e 100644
> --- a/src/wordpress.py
> +++ b/src/wordpress.py
> @@ -11,6 +11,18 @@ from yaml import safe_load
>  logger = logging.getLogger()
>  
>  
> +WORDPRESS_SECRETS = [
> +    "AUTH_KEY",
> +    "SECURE_AUTH_KEY",
> +    "LOGGED_IN_KEY",
> +    "NONCE_KEY",
> +    "AUTH_SALT",
> +    "SECURE_AUTH_SALT",
> +    "LOGGED_IN_SALT",
> +    "NONCE_SALT",
> +]

All these keys, when one would do... I wonder which ones should get cycled and when? I'm guessing individual deployments won't be using all of them, depending on how they get configured. Which is all fine.

> +
> +
>  def import_requests():
>      # Workaround until https://github.com/canonical/operator/issues/156 is fixed.
>      try:


-- 
https://code.launchpad.net/~tcuthbert/charm-k8s-wordpress/+git/charm-k8s-wordpress/+merge/385457
Your team Wordpress Charmers is requested to review the proposed merge of ~tcuthbert/charm-k8s-wordpress/+git/charm-k8s-wordpress:master into charm-k8s-wordpress:master.