feat: add dnf charm library #75

NucciTheBoss · 2023-03-01T19:12:57Z

Description

This pull request aims to add a dnf charm library offering to the operator_libs_linux collection of charm libraries. The dnf library will be used for charms that need to support both Ubuntu and RPM-based distributions (CentOS, Alma, etc.), as well as charms just intended to be deployed to just RPM-based Juju units. It comes with the standard bells and whistles needed to interact with dnf from a simple Python interface.

What still needs to be done

Documentation and registering the library on Charmhub. This pull request is mostly focused on just adding the initial code for a dnf charm library; do not want too many themes in the pull request review.

Notes

I updated the version of the setup-lxd GitHub Action to 0.1.0 instead of just using a commit hash. I also set the channel of LXD to 5.9/stable
I am using the cleantest library to provide the test instances for the dnf charm library. This way the library can be tested inside a RPM-based environment on GitHub Actions.
I added a separate tox env named integration-but-with-cleantest so that I did not mess with how the current integration tests are done. I added an extra step in the CI job for the new environment.

Notes: * This was done so that `config-manager` for add_repo would be available. `config-manager` is a plugin command outside of the base dnf distribution.

…ic LXD channel to use

…ntest

jnsgruk

A lot of this is just nits on style, but I do think in places we're over-complicating things and we probably need to consider if the added complexity is bringing much value over a more simplistic approach.

Remember that in the life of the charm, these methods are likely to be used only during install and upgrade-charm, which represents a very small portion of the charm's life, and never concurrently.

lib/charms/operator_libs_linux/v0/dnf.py

tests/integration/test_dnf.py

benhoyt

I think a lot of this is overly complex, and I'm worried about the "available" cache and the side effects done at import time. I've submitted a lot of comments to this effect. I hope they're constructive!

I think we should look at some use cases to determine what we actually need. And ask whether the cache is needed, or can we use something like dnf info instead.

tox.ini

lib/charms/operator_libs_linux/v0/dnf.py

tests/integration/test_dnf.py

benhoyt · 2023-03-02T09:16:08Z

Oh sorry, I spent a bunch of time reviewing this and just submitted my comments, but didn't see that @jnsgruk just had too. Looks like we have many of the same exact concerns. Sorry about the duplication!

NucciTheBoss · 2023-03-02T20:33:35Z

Thanks for reviewing this @jnsgruk and @benhoyt! I finished responding to most of your review comments. I am going to take dnf "back to the shop" and make some changes based on what has been discussed here. I will re-comment once I am done implementing my changes and ready for the next round of reviews.

Notes: * I removed the _Cache class for managing data about what packages are currently installed on the unit. * I removed the _MetaDNF class that checked if `dnf` was installed and brought in the `dnf-plugins-core` package. * I removed all the threading. * I removed the __getitem__ interface for retrieving package data.

NucciTheBoss · 2023-03-03T18:48:38Z

Okay @jnsgruk and @benhoyt, I have completed my initial passthrough for improving the dnf charm library. Big changes that I made to this new revision is that I removed _Cache and _MetaDNF completely. Let me know what you think of the updated version. In the meantime, I will be testing an EL9 base with Juju to see if I can get it working. If it works, I will issue some new commits bumping up to Python 3.8 (i.e. change PackageInfo to a dataclass rather than having a lot of boilerplate).

benhoyt

This is so much simpler, with any complexity that's left commensurate with what we're doing. Thanks! Just a few minor comments.

lib/charms/operator_libs_linux/v0/dnf.py

Notes: * Removed hashbang from top of file. * Renamed ExecutionError -> Error. * Removed UNKNOWN state as I find it to not be helpful. * Simplified `version()` and as such removed need for StringIO. * Refactored `upgrade()` so that upgrade can be performed even if no packages are specified. * Refactored how missing args are evaluated. * Refactored `fetch()` to be more resilient. i.e. handle regex match failure. * Removed `installed()` check from `_dnf()` in favor of just handling the thrown `FileNotFoundError` thrown by subprocess if desired executable is not found.

jnsgruk · 2023-03-06T21:25:29Z

Looking much, much simpler @NucciTheBoss - thanks! If you could clean up the unresolved conversations when you're done hacking we can take another pass at this and hopefully get it across the line?

NucciTheBoss · 2023-03-06T21:56:38Z

Looking much, much simpler @NucciTheBoss - thanks! If you could clean up the unresolved conversations when you're done hacking we can take another pass at this and hopefully get it across the line?

Sure can do @jnsgruk. Here is what I still need to do hacking wise:

Update to Python >= 3.8. I have verified that CentOS 9 Stream works with Juju so I am going to update to newer Python standards (i.e. dataclass rather than boilerplate @property).
Change the integration tests for dnf to how they are being done now.
Write some unit tests.
Need some of @benhoyt's feedback on my replies to his newest review comments. The update and purge methods are part of "my grand plan" for data-plane charms to work on both Ubuntu and RPM-based distributions.

lib/charms/operator_libs_linux/v0/dnf.py

Notes: * Removed the `purge(...)` and `update(...)` methods. * Add note to fetch about package name needing too exactly match installed package. * Changed `universal_newlines=True` to `text=True` in `_dnf(...)`. * Modified upgrade(...) so that there is not a condition check if no packages are passed. Change since Python is fine iterating over an empty args object.

Notes: * cleantest was removed after concluding discussion on this thread here: canonical#75 (comment). Cleantest can be reconsidered in a later PR, but for now we should just keep the tests to how they are being done now.

NucciTheBoss · 2023-03-07T19:56:36Z

Greetings @jnsgruk and @benhoyt - should be ready for the next round of reviews now. Here are the major changes you should be aware of now that I have published the new commits:

I removed the update() and purge(...) methods since they do not really exist in dnf.
I made PackageInfo a dataclass because I got CentOS 9 Stream to work as a base.
I removed cleantest from the integration tests and made some modifications to the current tox.ini file. TL;DR is that I also added an instance for CentOS 9 Stream to test dnf. See this thread on the Charmhub Mattermost for more information. I also removed the integration test for checking if dnf is installed since I can just mock with unittest.mock.
I added unit tests. Coverage is at 100%, but I did have to add a # pragma: no cover to add_repo(...). No matter how I patched the method, coverage would not mark it as covered. I think it had to do with trying to patch both subprocess.run and dnf.fetch(...). Luckily the integration tests for the library validate that the add_repo(...) method works!

Let me know what you thoughts are. We are getting closer!

benhoyt

Nice. Approving, but just one nit comment!

lib/charms/operator_libs_linux/v0/dnf.py

jnsgruk

One final thing: we need to register the lib name and add LIBID / LIBAPI / LIBPATCH before this can be uploaded :)

lib/charms/operator_libs_linux/v0/dnf.py

NucciTheBoss · 2023-03-08T15:01:48Z

Working on adding module level documentation for how to use the dnf charm library for Charmhub now.

jnsgruk · 2023-03-08T16:16:49Z

This is now published! https://charmhub.io/operator-libs-linux/libraries/dnf

Thanks @NucciTheBoss!

jamesbeedy · 2023-03-08T17:19:51Z

@NucciTheBoss - super! Nice work, and thank you!

NucciTheBoss added 9 commits February 28, 2023 16:53

enhance: Add initial dnf charm library

e44a31e

bugfix: There is not actually a dnf purge command

eb39972

bugfix: Install dnf-plugins-core when dnf is first defined.

0865332

Notes: * This was done so that `config-manager` for add_repo would be available. `config-manager` is a plugin command outside of the base dnf distribution.

enhance: Add tox env for running tests with cleantest

a4ac907

enhance: Add integration tests for dnf charm library

4dfccb7

enhance: Pin version of setup-lxd action to v0.1.0 and specify specif…

f931eb2

…ic LXD channel to use

enhance: Add pipeline section for running integration tests with clea…

465375a

…ntest

refactor: Switch rpm env to Alma Linux 9

bc5a24d

bugfix: Add ignore for test_dnf

1ae289d

NucciTheBoss mentioned this pull request Mar 1, 2023

Set workload version to version of SLURM service rather than git describe ... > version charmed-hpc/slurmdbd-operator#5

Closed

jnsgruk requested a review from benhoyt March 1, 2023 19:49

jnsgruk requested changes Mar 2, 2023

View reviewed changes

benhoyt requested changes Mar 2, 2023

View reviewed changes

NucciTheBoss added 3 commits March 3, 2023 10:47

refactor: Simplify tox env name

5fea119

refactor: Update tests to reflect new dnf API

6a2b799

NucciTheBoss mentioned this pull request Mar 3, 2023

Improve testing of charm libraries #76

Open

NucciTheBoss requested review from jnsgruk and benhoyt and removed request for jnsgruk and benhoyt March 3, 2023 22:34

benhoyt reviewed Mar 6, 2023

View reviewed changes

lib/charms/operator_libs_linux/v0/dnf.py Outdated Show resolved Hide resolved

NucciTheBoss added 6 commits March 7, 2023 10:15

refactor: Change PackageInfo to dataclass

b55e859

refactor: Make integration test similar to other tests

8d1ccc5

Notes: * cleantest was removed after concluding discussion on this thread here: canonical#75 (comment). Cleantest can be reconsidered in a later PR, but for now we should just keep the tests to how they are being done now.

refactor: Update CI and tox to use CentOS 9 Stream image for testing dnf

75204be

refactor: Fix spelling error in install docstring

0f14712

enhance: Add unit tests

5c47c80

NucciTheBoss requested a review from benhoyt March 7, 2023 19:47

benhoyt approved these changes Mar 8, 2023

View reviewed changes

lib/charms/operator_libs_linux/v0/dnf.py Show resolved Hide resolved

lib/charms/operator_libs_linux/v0/dnf.py Outdated Show resolved Hide resolved

jnsgruk requested changes Mar 8, 2023

View reviewed changes

lib/charms/operator_libs_linux/v0/dnf.py Outdated Show resolved Hide resolved

NucciTheBoss and others added 3 commits March 8, 2023 09:24

refactor: Drop type annotations in docstrings

9f137e0

refactor: Return package info rather than recapture error

f75e25f

feat: register the dnf lib with charmhub

07dbc8f

docs: Add module-level docstring for Charmhub documentation

50a9aaf

jnsgruk approved these changes Mar 8, 2023

View reviewed changes

jnsgruk changed the title ~~Add dnf charm library~~ feat: add dnf charm library Mar 8, 2023

jnsgruk merged commit 4f8ed01 into canonical:main Mar 8, 2023

NucciTheBoss deleted the dnf branch November 19, 2024 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add dnf charm library #75

feat: add dnf charm library #75

NucciTheBoss commented Mar 1, 2023

jnsgruk left a comment

benhoyt left a comment

benhoyt commented Mar 2, 2023

NucciTheBoss commented Mar 2, 2023

NucciTheBoss commented Mar 3, 2023

benhoyt left a comment

jnsgruk commented Mar 6, 2023

NucciTheBoss commented Mar 6, 2023

NucciTheBoss commented Mar 7, 2023

benhoyt left a comment

jnsgruk left a comment

NucciTheBoss commented Mar 8, 2023

jnsgruk commented Mar 8, 2023

jamesbeedy commented Mar 8, 2023

feat: add dnf charm library #75

feat: add dnf charm library #75

Conversation

NucciTheBoss commented Mar 1, 2023

Description

What still needs to be done

Notes

jnsgruk left a comment

Choose a reason for hiding this comment

benhoyt left a comment

Choose a reason for hiding this comment

benhoyt commented Mar 2, 2023

NucciTheBoss commented Mar 2, 2023

NucciTheBoss commented Mar 3, 2023

benhoyt left a comment

Choose a reason for hiding this comment

jnsgruk commented Mar 6, 2023

NucciTheBoss commented Mar 6, 2023

NucciTheBoss commented Mar 7, 2023

benhoyt left a comment

Choose a reason for hiding this comment

jnsgruk left a comment

Choose a reason for hiding this comment

NucciTheBoss commented Mar 8, 2023

jnsgruk commented Mar 8, 2023

jamesbeedy commented Mar 8, 2023