juju / python-libjuju

Python library for the Juju API
Apache License 2.0
57 stars 93 forks source link

Make consume respect the controller name in the url #1038

Closed Aflynn50 closed 3 months ago

Aflynn50 commented 3 months ago

Description

consume takes a controller_name argument. This is not needed and should be automatically generated from the offer url. A deprecation warning is logged and the variable is not used.

Also update offer and consume integration tests to no longer be skipped.

This helps avoid #1031

QA Steps

juju bootstrap lxd c1
juju add-model offerer
juju deploy juju-qa-dummy-source dummy-source
juju offer dummy-source:sink
juju bootstrap lxd c2
python -m asyncio

# In REPL
from juju import model
m = model.Model()
await m.connect()
await m.consume("c1:admin/offerer.dummy-source")

# Exit REPL
juju status
# Confirm SAAS exists
juju remove-saas dummy-source
# Confirm removal
python -m asyncio

# In REPL
from juju import model
m = model.Model()
await m.connect()
await m.consume("admin/offerer.dummy-source", controller_name="c1")

# Exit REPL
juju status
# Confirm SAAS exists
juju remove-saas dummy-source
# Confirm removal
python -m asyncio

# In REPL
from juju import model
m = model.Model()
await m.connect()
await m.consume("c1:admin/offerer.dummy-source", controller_name="c1")

All CI tests need to pass.

Aflynn50 commented 3 months ago

Code looks good. Could you please check if there's a test with a controller name in the endpoint url? If not we should add that (for parse_offer_url, or _get_source_api) I'd be ok if it's a mocked test within tests/unit if coming up with a sensible integration test is hard. We just need to ensure the capability is there for future. Hope that makes sense.

Also looks like we need a rebase.

I've added a unit test which checks the consume behavior, though its uses a lot of mocks.

I've also changed it so you can use the controller_name argument as expected, just to ensure that we don't break anything with this.

Aflynn50 commented 3 months ago

/merge