Reduce functionality, rewrite from scratch - #21
Conversation
d6508c5 to
971e3b4
Compare
tacerus
changed the title
|
I deem this ready, however I understand if it's deemed too breaking; I think implementation of repositories and registry logins will be interesting for some, but it needs to happen in pyhelm3 first. The functionality implemented so far works well with OCI repositories. |
|
Disclaimer: I neither use Helm nor the modules in here, only skimmed the code during the migration. In general, I agree with everything you wrote. I'd even question if anyone uses them in their current iteration or if they work with recent Helm versions since the relevant code has not been touched for ages and relies on generating CLI commands only, despite claiming to have a Python lib dependency: saltext-helm/src/saltext/helm/modules/helm.py Lines 4 to 6 in a962dc1 I'll try to do a proper code review next week. Would you be fine with becoming a maintainer of this repo after merging your rewrite? |
|
Indeed, that comment I found odd too (there wasn't anything in the history about it having used that library either). Thanks, and sure, I'd be happy to, just need to learn how to work with the copier stuff. :-) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21 +/- ##
==========================================
+ Coverage 82.92% 83.48% +0.56%
==========================================
Files 9 9
Lines 808 333 -475
Branches 100 29 -71
==========================================
- Hits 670 278 -392
+ Misses 107 46 -61
+ Partials 31 9 -22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Regarding
Is there a more standard way to document additional minion configuration for modules? I couldn't find any in salt or saltext documentation. |
Awesome!
Mostly, you won't need to work with Copier itself since we have Renovate take care of template update PRs inside this org. In general, the template docs should tell you everything you might need to know: https://salt-extensions.github.io/salt-extension-copier/ Something to keep in mind: Try to keep changes to boilerplate lines, especially those that tend to change often, to a minimum to avoid having to repeatedly resolve merge conflicts when these Renovate PRs happen. That applies to most pregenerated files other than modules and those matching
Since https://github.com/salt-extensions/saltext-vault/blob/dfec8fad4fbb7232956de89fef6d7e7acd7a02eb/docs/_ext/vault/vaultdomain.py (Note: The linked example usage is in MyST format, which cannot be used inside Python docstrings - but it works in RST too) * Edit: It might be possible to use the inbuilt .. conf_minion:: helm.kubeconfig
helm.kubeconfig
Your description
Some text referencing :conf_minion:`this new config <helm.kubeconfig>` |
|
Thanks for the pointers! The vconf example looks interesting but I think is a bit too complex here as I don't need to reference the options often ... I found the redis module to use a 
|
It does work with recent helm versions - I'm a user :) . We use the helm cli states/modules to bootstrap an on-prem rancher plus argocd cluster - I am very relieved in the renewed interest in the module, things were looking pretty dire as you mentioned. Over time I clobbered on a few extra basic 'features' to the cli version in our _states/_modules folders, primarily:
Our usage has since evolved to where we don't even need items 1 or 2 anymore, so I would be fine testing and cutting over to this implementation when I can find the time. The idempotency is also greatly desired - as you could probably guess in its current state, the helm revision counter increments every time the state is applied. |
|
Great to know there's more interest. :-) Specifying an external values file is currently not supported, however my hope is that it will not be needed anymore, because with pyhelm3 the provided values are now passed to |
|
I tested this with some real data now (including "complex" values such as X509 certificates, special characters and Go template strings), all was applied correctly through |
|
There are some improvements I notice would be nice - like adding an absent function and implementing dictdiffer for more useful changes output. But I will leave those for a later PR as to not make this (working) diff any larger. |
lkubb
left a comment
lkubb
left a comment
There was a problem hiding this comment.
Many thanks for taking the time to rewrite this extension and very sorry about the significant delay.
This looks pretty good. Most of the comments are personal suggestions or nits, you're free to decide whether to act on them or not.
One more general nit: Some of the function parameters could use descriptions imho.
Cheers!
| if install_test_requirements: | ||
| install_extras.append("tests") | ||
|
|
||
| # https://github.com/azimuth-cloud/pyhelm3/issues/28 |
There was a problem hiding this comment.
question: There is no simple way to make the latest released version work? Otherwise we'll have to tell users they need to install it manually from the repository...
Sadly PyPI does not allow direct references like pyhelm3 @ git+https://github.com/azimuth-cloud/pyhelm3@c2f6bb9cbe4805010dc0b5aa0c57ad3081f70f45 in the project dependencies.
There was a problem hiding this comment.
The released version is quite old and misses support for OCI containers: azimuth-cloud/pyhelm3@62922cd, so it would not behave correctly with the features implemented here...
I use the extension and all dependencies through distribution packages, so it's not an issue for me because I just build pyhelm3 from Git sources anyways.
I understand it's not convenient for users relying on online pip, unfortunately not sure how to improve it.
There was a problem hiding this comment.
I could check if it's possible to publish a fork to PyPi, but I have not done that before.
There was a problem hiding this comment.
Ah, I don't think that's necessary, just add the requirement to the installation docs then. It should be trivial for users to add a pip.installed in their states.
| return | ||
|
|
||
| return result | ||
| global c # pylint: disable=global-statement # needed to share for command line use |
There was a problem hiding this comment.
question: Could we use __context__ instead? With the Salt loader shenanigans, this seems flaky.
Like this:
CKEY = "_helm_client"
def _client():
config = {
opt: __opts__.get(f"helm.{opt}") for opt in ("kubeconfig", "kubecontext", "executable")
}
config_key = (config["kubeconfig"], config["kubecontext"], config["executable"])
if CKEY not in __context__:
__context__[CKEY] = {}
if config_key not in __context__[CKEY]:
__context__[CKEY][config_key] = Client(
**{opt: val for opt, val in config.items() if val is not None}
)
return __context__[CKEY][config_key]There was a problem hiding this comment.
Interesting idea (and I would like to avoid the global) unfortunately I get NameError: name '__context__' is not defined. Is this supposed to work under saltext?
There was a problem hiding this comment.
Yes, but it's not yet defined when __init__() is run, if you tried to put the logic there. It should definitely work in a _client() function that's run on demand.
If it's about the test suite, adding the configure_loader_modules fixture should help.
Edit: Unsure if a loader is already set during __init__(), but you could try explicitly importing the corresponding NamedLoaderContext if the logic really needs to happen in __init__:
from salt.loader.dunders import __context__| def test_list_releases(fake_output): | ||
| def side(command): | ||
| if command[0] == "get": | ||
| return fake_output.get(command[0] + "_" + command[1]) | ||
|
|
||
| return fake_output.get(command[0]) | ||
|
|
||
| with patch.object(pyhelm3.command.Command, "run", side_effect=side): | ||
| res = helm.list_releases() | ||
|
|
||
| # only test first element here, we don't mock the individual status output of all releases in the mocked list output | ||
| assert isinstance(res, list) | ||
| assert res[0] == RELEASE |
There was a problem hiding this comment.
| def test_list_releases(fake_output): | |
| def side(command): | |
| if command[0] == "get": | |
| return fake_output.get(command[0] + "_" + command[1]) | |
| return fake_output.get(command[0]) | |
| with patch.object(pyhelm3.command.Command, "run", side_effect=side): | |
| res = helm.list_releases() | |
| # only test first element here, we don't mock the individual status output of all releases in the mocked list output | |
| assert isinstance(res, list) | |
| assert res[0] == RELEASE | |
| @pytest.fixture | |
| def run_mock(fake_output): | |
| def _run(command, *args, **kwargs): | |
| if command[0] == "show" or command[0] == "get": | |
| return fake_output.get(command[0] + "_" + command[1]) | |
| return fake_output.get(command[0]) | |
| with patch.object(pyhelm3.command.Command, "run", side_effect=_run) as mock: | |
| yield mock | |
| @pytest.mark.usefixtures("run_mock") | |
| def test_list_releases(release): | |
| res = helm.list_releases() | |
| # only test first element here, we don't mock the individual status output of all releases in the mocked list output | |
| assert isinstance(res, list) | |
| assert res[0] == release |
question: Is there an issue with generically patching the run function? If this works, you can get rid of many lines in this test module.
There was a problem hiding this comment.
Done, tests which need it, now only require the fake_run fixture. Note I did not set it to autouse, as the test for exception handling needs to not use it.
|
|
||
| kvflags | ||
| (dict) Flags in argument of the command with values. ex: {'v': 2, '--v': 4} | ||
| return run(_install_or_upgrade_release(release_name, chart, values, **l)) |
There was a problem hiding this comment.
nit: Unsure if this should return the raw object, unless it's not trivial to render it to a dict by default.
There was a problem hiding this comment.
Indeed it shouldn't, as this does not render on the CLI. I made it a dict now.
@lkubb, thank you for reviewing, and likewise apologies for the delay. |
ea4afdc to
9e5f617
Compare
The existing code seemed unmaintained and had design issues - for example, the state functions were not only not idempotent, they also did not have any logic to attempt idempotency. Further, the custom parsing logic made attempts to improve the situation difficult, and argument handling was not very idiomatic (like expecting a list of colon separated keys instead of a proper dictionary as chart values). Use the opportunity of lack of activity in the repository to rewrite the modules from scratch. The new implementation uses pyhelm3 - while this adds another dependency, working with the code should become much simpler as unified classes exist over Helm output and the parsing logic is shared with a wider community. Not all functionality as before is implemented, as more will require prior implementation in pyhelm3. However, managing releases idempotently is deemed most imporant functionality, hence dropping the additional features to begin with is deemed a fair compromise. This is by design a breaking change with no intention of backwards compatibility. Signed-off-by: Georg Pfuetzenreuter <georg.pfuetzenreuter@suse.com>
3 participants
The existing code seemed unmaintained and had design issues - for
example, the state functions were not only not idempotent, they also did
not have any logic to attempt idempotency. Further, the custom parsing
logic made attempts to improve the situation difficult, and argument
handling was not very idiomatic (like expecting a list of colon separated
keys instead of a proper dictionary as chart values).
Use the opportunity of lack of activity in the repository to rewrite the
modules from scratch. The new implementation uses pyhelm3 - while this
adds another dependency, working with the code should become much
simpler as unified classes exist over Helm output and the parsing logic
is shared with a wider community.
Not all functionality as before is implemented, as more will require
prior implementation in pyhelm3. However, managing releases idempotently
is deemed most imporant functionality, hence dropping the additional
features to begin with is deemed a fair compromise.
This is by design a breaking change with no intention of backwards
compatibility.
For reviewing, it's probably easier to read the individual files instead of the diff.
What does this PR do?
Rewrite the code base to a smaller but more modern implementation.
What issues does this PR fix or reference?
Fixes: n/a
Previous Behavior
No idempotent handling of releases, complicated code base.
New Behavior
Idempotent handling of releases, streamlined code base.
Merge requirements satisfied?
[NOTICE] Bug fixes or new features require tests.
Commits signed with GPG?
Yes