Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add missing dependency (pylibjuju) #120

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tonyandrewmeyer
Copy link

@tonyandrewmeyer tonyandrewmeyer commented Dec 10, 2023

The tests use importlib.metadata to get the installed version of pylibjuju (tests/conftest.py):

    if juju_version := os.environ.get("LIBJUJU_VERSION_SPECIFIER"):
        juju_version.replace("==", "")
        juju_version = juju_version[2:].split(".")[0]
    else:
        juju_version = version("juju")

However, pylibjuju is not installed as part of the requirements. This means that all tests fail with an error like:

ERROR tests/unit/test_data_interfaces.py::TestDatabaseProvides::test_database_requested_event - importlib.metadata.PackageNotFoundError: No package metadata was found for juju

This is fixed by installing pylibjuju for the unit environment as well as integration. Alternatively, the version("juju") could be wrapped in a try/except, and you could fall back to a hard-coded version?

Copy link
Contributor

@shayancanonical shayancanonical left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL: tox env var substitutions. LGTM!

Copy link
Contributor

@deusebio deusebio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I would just update the reference to the juju to something more recent than 2.9.x

@tonyandrewmeyer
Copy link
Author

Looks good! I would just update the reference to the juju to something more recent than 2.9.x

It's the same as you have for your integration tests (I assumed you'd want it to match), but happy to change it to whatever you'd prefer.

@deusebio
Copy link
Contributor

We tests a number of versions, among which 2.9.42.4 which is the oldest. I would just keep on testing on CI on that one, but I would move the default reference to latest, e.g. 3.5.0.0. Otherwise the reference would already seem out of date.

I don't think that should break anything, transitive dependencies included in tox.ini are not resolved in charmlib anyway... :D

tox.ini Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants