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

Relation data wrapper #82

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

Conversation

PietroPasotti
Copy link
Contributor

@PietroPasotti PietroPasotti commented May 31, 2022

Added a get_relation_data method to OpsTest, allowing itest code to easily get a hold of databag contents for both sides of a live relation (where two units and one endpoint per unit are involved).

Main idea:

relation_data = await ops_test.get_relation_data(requirer_endpoint='prometheus/0:ingress', provider_endpoint='traefik/0:ingress')
assert relation_data.provider.application_data == {'foo': 'bar', 'baz': 42}

The code is copy-pasted from https://github.com/PietroPasotti/jhack utils show-relation, a utility to visualize relation databags.

Questions for the reviewers:

  • should I integrate with the existing machinery in OpsTest? e.g. Should I use OpsTest.juju instead of 'raw' Popen calls?
  • I thought it sensible to put the code in a separate file, is that ok? Separate submodule?

Fixes #81

@ca-scribner
Copy link
Contributor

Did you mean to include the jetbrains folder in the PR?

@PietroPasotti
Copy link
Contributor Author

I did not :D

@ca-scribner
Copy link
Contributor

I like this feature, it would be very helpful.

Re: where it fits best in the package: I don't have a great feel for it but @addyess you've been in this package a lot lately do you have an opinion
Re: 'raw' Popen: My gut says if there's a non-raw way of doing it, that would be preferred. I think the ideal would be we're not hitting the juju cli. But again, I don't have a good feel for it



async def get_relation_data(
provider_endpoint: str, requirer_endpoint: str, include_juju_keys: bool = False
Copy link
Contributor

Choose a reason for hiding this comment

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

Does anywhere else use the provider endpoint like this as an argument? I can't think of any, but I probably have forgotten or never known in the first place. I ask just because on first read this felt odd to see in charm code, but it might just be because I haven't dealt with similar things

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah so this is my own take on the matter. Not sure how else to expose this.
Issue is: we need an app name, a unit number, and a endpoint name. It felt that maintaining the 'juju cli' syntax was the least disruptive option, so unit_name/unit_number:endpoint_name.
But I'd be willing to unpack this into a more semantically expressive object, a NamedTuple('unit, number, endpoint') or something like that. Or maybe we should be accepting juju.model.Unit etc... but I wanted to avoid forcing the user to fetch a specific unit instance when the app name is usually much easier to get in itests...

@addyess
Copy link
Member

addyess commented Jun 7, 2022

sorry i'm late to the party but this doesn't feel like it should be in pytest-operator, but in python libjuju somewhere. Analysis of a model's relation data seems to live in that world.

It could be exposed through ops_test.model as that represents the juju Model object. I'd think that's where this ought to belong?

I'm looking to see if that "feature" may already exist there.

@addyess
Copy link
Member

addyess commented Jun 7, 2022

yeah, it doesn't seem like python libjuju has this feature, but i'd expect it to be in the Endpoint class

for _, relation in ops_test.model.relations:
    for endpoint in relation.endpoints:
        # probe around in here

there are some convenience methods in libjuju, but i haven't spent more than 10min digging around in here.

@marcoppenheimer
Copy link

Just to +1 here.

I tried looking into model.relations but couldn't grab any data from it.

My solution (which I'd rather not have to do) was:

def get_password(model_full_name):
    show_unit = check_output(
        f"JUJU_MODEL={model_full_name} juju show-unit {APP_NAME}/0",
        stderr=PIPE,
        shell=True,
        universal_newlines=True,
    )
    response = yaml.safe_load(show_unit)
    password = response[f"{APP_NAME}/0"]["relation-info"][0]["application-data"]["super_password"]
    logger.info(f"{password=}")
    return password

@addyess
Copy link
Member

addyess commented Aug 3, 2023

@PietroPasotti thanks for this contribution. @ca-scribner I'm satisfied with this feature now that it's got some tests on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Means of testing relation data
4 participants