Update bundle related methods#99
Update bundle related methods#99NucciTheBoss wants to merge 4 commits intocharmed-kubernetes:mainfrom
Conversation
Notes: * What was wrong? 1. In `deploy_bundle`, I needed to convert the Path object for the bundle file to a string. If you do not convert to string first, Python will throw errors about trying to pass a Path object instead of a string to subprocess. 2. In `render_bundle`, I needed to update the regex to allow files with just the extension .j2 and .tmpl instead of being prefixed with .yaml. If I did not do that, the function would try to perform string operations on the Path object which would throw type errors.
| if serial: | ||
| cmd += ["--serial"] | ||
| await self.run(*cmd, check=True) | ||
| bundle: Union[str, os.PathLike], |
There was a problem hiding this comment.
this changes the signature of this method. We'll need to major rev the package and update the docs accordingly
There was a problem hiding this comment.
I'm not actually convinced that the signature actually needs to change. The method signature and docstrings of an API essentially define the contract for method itself. The existing contract is fairly vague in the specification, but clear in the parameters. This change can largely (if not wholly) still honor the contract of the method without making a breaking API change. An additional advantage/improvement would be to further refine and clarify the method contract in the docstrings.
As there are existing users of this library, if the API is changed then it will break a downstream consumer of the library. Since the method contract can be upheld with the existing signature, it seems better to me to not change the signature and subsequently not break downstream consumers.
Just my $0.02
| async def deploy_bundle( | ||
| self, | ||
| bundle: Optional[str] = None, | ||
| build: bool = True, | ||
| serial: bool = False, | ||
| extra_args: Iterable[str] = (), | ||
| ): |
There was a problem hiding this comment.
this method now has a new signature and will also need the documentation updated
| Any, | ||
| Dict, |
There was a problem hiding this comment.
I feel like this is here to create a type like Dict[str,Any] in quite a few places to indicate a a dict for templating context
Maybe its worth defining a local type like
Context = Dict[str, Any]to use when we intend to render some context
technically jinja2 is very loose on it's typing.
effectively...
def render(*args: Any, **kwargs: Any):
...
|
Hi there @addyess - thank you for the initial round of reviews. I will work through them as I have the time 😄 Either way, we should get folks off of using |
Cool, i can agree with getting off |
4 participants
Description
This pull request aims to fix the issues that I identified in issue #98. Here is the summary of modifications that I made below:
build_bundlemethod to usecharmcraft packrather thanjuju-bundle. It now returns a Path object that points to where the built bundle zip archive is stored.build_bundlesmethod to build bundles asynchronously .deploy_bundleto usejuju deploy ...rather thanjuju-bundle.render_bundleto also accept the .tmpl file extension. I also enhanced the robustness of the expression. The original regex would match inputs such asxyamlij2,Byaml, etc since "." means any character. I modified it so that the regex would only accept the "." character.Related issues
juju-bundlecauses breakage when callingdeploy_bundle#98Miscellaneous
I updated the
.gitignorefile to ignore the.ideadirectory. I made this modification since I use PyCharm. I also added a hashbang and copyright information to the top ofplugin.py.