wordpress-charmers team mailing list archive

[Tom Haddon <tom.haddon@xxxxxxxxxxxxx>]

I think I'm still a bit confused as to what the long term plan for this branch is. How will this be possible via charm config? fetcher.py is a script run to build the OCI image, so I don't see how we can use charm config here, or are you saying that we'll eventually also call it at run time within the running containers? This seems like we'd run into issues with connecting to places to download the plugins from. It also means we'd have to run this every time we spin up a new container (e.g. scale out, or if a pod gets rescheduled by k8s).

The relationship between charm config and plugins is an interesting one though, as obviously we don't want to build images with plugins that aren't needed, but then that means if you want to update which plugins you run, you'd potentially need a different image. This potentially needs wider discussion before we determine a pattern that makes sense.

Let's hold on this change just now and I'll try and have the relevant discussions about this and determine our path forward.

Diff comments:

> diff --git a/image-builder/src/fetcher.py b/image-builder/src/fetcher.py
> index b7085f2..e9d425f 100755
> --- a/image-builder/src/fetcher.py
> +++ b/image-builder/src/fetcher.py
> @@ -98,11 +102,11 @@ def get_plugins(zip_plugins, branch_plugins):
>          current_zip = current_zip + 1
>          print('Downloading {} of {} zipped plugins: {} ...'.format(current_zip, total_zips, zip_plugin))
>          url = 'https://downloads.wordpress.org/plugin/{}.latest-stable.zip'.format(zip_plugin)
> -        file_name = os.path.join(os.getcwd(), 'files/plugins', os.path.basename(url))
> +        file_name = os.path.join(os.getcwd(), 'plugins', os.path.basename(url))

I'm not sure I understand why it's okay to rename this directory. Can you clarify?

>          with urllib.request.urlopen(url) as response, open(file_name, 'wb') as out_file:
>              shutil.copyfileobj(response, out_file)
>          with zipfile.ZipFile(file_name, 'r') as zip_ref:
> -            zip_ref.extractall(os.path.join(os.getcwd(), 'files/plugins'))
> +            zip_ref.extractall(os.path.join(os.getcwd(), 'plugins'))
>          os.remove(file_name)
>  
>      total_branches = len(branch_plugins)
> @@ -150,6 +154,93 @@ def get_themes(branch_themes):
>          _ = subprocess.check_output(cmd, universal_newlines=True, stderr=subprocess.STDOUT)
>  
>  
> -if __name__ == '__main__':
> +class Plugin:
> +    name: str
> +    _protocol: str
> +    _url: str
> +
> +    def __init__(self, name: str, url: str):
> +        self.name = name
> +        self.url = url
> +
> +    def __str__(self) -> str:
> +        return self.name
> +
> +    def __repr__(self) -> str:
> +        kwargs = {"name": self.name, "url": self.url}
> +        return "<Plugin>(name={name}, url={url})".format(**kwargs)
> +
> +    @property
> +    def url(self):
> +        return self._url
> +
> +    @url.setter
> +    def url(self, url: str):
> +        u = urlparse(url)
> +        self._url = u.geturl()
> +        self._protocol = u.scheme.split("+")[0]
> +        self._protocol = self._protocol.replace("https", "git")
> +
> +    def is_available(self):
> +        print("NOT IMPLEMENTED")
> +
> +    def is_bzr(self):
> +        return self._protocol == "lp"
> +
> +    def is_git(self):
> +        return self._protocol == "git"
> +
> +    def sync(self, dest: str):
> +        self.__make_dest(dest)
> +        failed = self.__call_sync(os.path.join(dest, self.name))
> +        if failed:
> +            raise RuntimeError("failed to sync plugin: {0}".format(self))
> +
> +    def __make_dest(self, dest: str):
> +        os.makedirs(dest, mode=0o755, exist_ok=True)
> +
> +    def __build_cmd(self, dest: str) -> List[str]:
> +        cmds = {
> +            "git": ["git", "clone", self.url, dest],
> +            "lp": ["bzr", "branch", self.url, dest],
> +        }
> +        return cmds[self._protocol]
> +
> +    def __call_sync(self, dest: str, verbose=True):
> +        subprocess_kwargs = {}
> +        subprocess_kwargs["universal_newlines"] = True
> +        if verbose:
> +            subprocess_kwargs["stderr"] = subprocess.STDOUT
> +
> +        rv = subprocess.run(self.__build_cmd(dest), **subprocess_kwargs)
> +        return rv.returncode
> +
> +
> +def sync_additional_plugins(additional_plugins: Mapping[str, str]):
> +    """
> +    Sync any additional WordPress plugins specified from the Container
> +    environment variable: WORDPRESS_ADDITIONAL_PLUGINS.
> +
> +    WORDPRESS_ADDITIONAL_PLUGINS must be a valid YAML formatted map string.
> +    """
> +    dest_path = os.path.join(os.getcwd(), "plugins")
> +    total_plugins = len(additional_plugins)
> +    count = 0
> +    for name, url in additional_plugins.items():
> +        count += 1
> +        plugin = Plugin(name, url)
> +        print("Syncing {0} of {1} WordPress plugins: {2} ...".format(count, total_plugins, plugin))
> +        plugin.sync(dest_path)
> +
> +
> +if __name__ == "__main__":
> +    if "WORDPRESS_ADDITIONAL_PLUGINS" in os.environ.keys():
> +        additional_plugins = os.environ["WORDPRESS_ADDITIONAL_PLUGINS"]

`additional_plugins = os.environ.get("WORDPRESS_ADDITIONAL_PLUGINS")` would save one level of indentation here.

> +        if additional_plugins:
> +            plugins = safe_load(additional_plugins.encode("utf-8"))
> +            sync_additional_plugins(plugins)
> +            # TODO: Script should have command-line flags instead of this.
> +            print("DEBUG: we are running inside a container, no need to sync base plugins")
> +        sys.exit(0)
>      get_plugins(zip_plugins_to_get, branch_plugins_to_get)
>      get_themes(branch_themes_to_get)


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