Review: Approve
Yup
--
https://code.launchpad.net/~mthaddon/charm-k8s-wordpress/+git/charm-k8s-wordpress/+merge/396800
Your team Wordpress Charmers is requested to review the proposed merge of
~mthaddon/charm-k8s-wordpress/+git/charm-k8s-wordpress:maridb-charmhub into
Review: Approve
Yup. Minor comment inline.
Diff comments:
> diff --git a/tests/unit/test_wordpress.py b/tests/unit/test_wordpress.py
> index 92712e7..47cf8be 100644
> --- a/tests/unit/test_wordpress.py
> +++ b/tests/unit/test_wordpress.py
> @@ -82,6 +82,15 @@ class
Review: Approve
Ok. I guess the issue here is that we were relying on Juju setting the active
status for us, and it won't do that unless it has done sort of action that
warrants it (such as spinning up new pods with a new spec). I don't think it is
clear when and why Juju updates the workload
The proposal to merge ~stub/charm-k8s-wordpress:ops-lib-mysql into
charm-k8s-wordpress:master has been updated.
Status: Needs review => Approved
For more details, see:
https://code.launchpad.net/~stub/charm-k8s-wordpress/+git/charm-k8s-wordpress/+merge/396227
--
Your team Wordpress
https://github.com/canonical/ops-lib-mysql
--
https://code.launchpad.net/~stub/charm-k8s-wordpress/+git/charm-k8s-wordpress/+merge/396227
Your team Wordpress Charmers is requested to review the proposed merge of
~stub/charm-k8s-wordpress:ops-lib-mysql into charm-k8s-wordpress:master.
--
Stuart Bishop has proposed merging ~stub/charm-k8s-wordpress:ops-lib-mysql into
charm-k8s-wordpress:master.
Commit message:
Switch to ops-lib-mysql
Move the embedded mysql endpoint implementation to an external
library for sharing with other charms.
Requested reviews:
Wordpress Charmers
Also adds a test
--
https://code.launchpad.net/~stub/charm-k8s-wordpress/+git/charm-k8s-wordpress/+merge/396227
Your team Wordpress Charmers is requested to review the proposed merge of
~stub/charm-k8s-wordpress:ops-lib-mysql into charm-k8s-wordpress:master.
--
Mailing list:
The proposal to merge ~stub/charm-k8s-wordpress:mysql-relation into
charm-k8s-wordpress:master has been updated.
Status: Approved => Merged
For more details, see:
https://code.launchpad.net/~stub/charm-k8s-wordpress/+git/charm-k8s-wordpress/+merge/395826
--
Your team Wordpress Charmers is
Review: Approve
Yup, all good.
--
https://code.launchpad.net/~mthaddon/charm-k8s-wordpress/+git/charm-k8s-wordpress/+merge/396062
Your team Wordpress Charmers is requested to review the proposed merge of
~mthaddon/charm-k8s-wordpress/+git/charm-k8s-wordpress:optional-tls into
Review: Approve
Looks good!
Diff comments:
> diff --git a/src/charm.py b/src/charm.py
> index edade47..1c2f1de 100755
> --- a/src/charm.py
> +++ b/src/charm.py
> @@ -146,7 +152,8 @@ class WordpressCharm(CharmBase):
> msg = "Wordpress needs configuration"
>
Review: Approve
Yup, all good.
--
https://code.launchpad.net/~tcuthbert/charm-k8s-wordpress/+git/charm-k8s-wordpress/+merge/385455
Your team Wordpress Charmers is subscribed to branch charm-k8s-wordpress:master.
--
Mailing list: https://launchpad.net/~wordpress-charmers
Post to :
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
> ---
Review: Approve
Seems fine, apart from being a shell script, which we have a general policy
against because they grow and devour future travellers. Given it is calling
plugin_handler.py, I even know there is an interpreter already on the image :)
Not for this branch, but worth adding a task to
Review: Approve
Looks good
--
https://code.launchpad.net/~mthaddon/charm-k8s-wordpress/+git/charm-k8s-wordpress/+merge/383718
Your team Wordpress Charmers is requested to review the proposed merge of
~mthaddon/charm-k8s-wordpress/+git/charm-k8s-wordpress:typo-fix into
Review: Approve
This all looks good. Minor comment about indentation, which looks odd to me but
not actually wrong.
Diff comments:
> diff --git a/tests/unit/test_charm.py b/tests/unit/test_charm.py
> new file mode 100644
> index 000..19d0ce3
> --- /dev/null
> +++ b/tests/unit/test_charm.py
Review: Approve
Looks fine. A couple of inline comments, but should be obvious enough.
Diff comments:
> diff --git a/src/charm.py b/src/charm.py
> new file mode 100755
> index 000..8ff478a
> --- /dev/null
> +++ b/src/charm.py
> @@ -0,0 +1,342 @@
> +#!/usr/bin/env python3
> +
> +import io
>
Review: Needs Fixing
Diff comments:
> diff --git a/src/charm.py b/src/charm.py
> new file mode 100755
> index 000..5204372
> --- /dev/null
> +++ b/src/charm.py
> @@ -0,0 +1,346 @@
> +#!/usr/bin/env python3
> +
> +import io
> +import re
> +import secrets
> +import string
> +import
Review: Needs Fixing
--
https://code.launchpad.net/~tcuthbert/charm-k8s-wordpress/+git/charm-k8s-wordpress/+merge/381502
Your team Wordpress Charmers is requested to review the proposed merge of
~tcuthbert/charm-k8s-wordpress/+git/charm-k8s-wordpress:operator into
charm-k8s-wordpress:master.
Review: Approve
Discussions indicate it doesn't matter if the script times out or not, as a
failure will not escalate the pod to a failed state or inform Juju. Something
else needs to happen to catch the case where wordpress comes up but the
configuration script fails.
--
Review: Needs Information
Code looks fine.
Thanks for keeping what you can Python. I can't really review the PHP chunks,
which seem to be the least worst solution.
I agree with other commentators that it may be best for wait_for_wordpress to
timeout, as I expect it is better for the pod spin
Review: Approve
This looks good. One comment inline for some minor code shuffling. Try not to
include black code reformats together with other reviews.
Diff comments:
> diff --git a/reactive/wordpress.py b/reactive/wordpress.py
> index 39ea6dc..e47f971 100644
> --- a/reactive/wordpress.py
>
Review: Approve
Looks good. Makefile lint target needs to call the tox rule you have created,
per Tom's comment.
--
https://code.launchpad.net/~barryprice/charm-k8s-wordpress/+git/charm-k8s-wordpress/+merge/376327
Your team Wordpress Charmers is subscribed to branch charm-k8s-wordpress:master.
22 matches
Mail list logo
