fix parsing of storage constraints

[luissimas]

Description

Fixes: #1052. Parses the storage constraints when deploying applications. Before this change the storage constraints where not parsing, resulting in an error when deploying bundles that contained applications with storage definitions.

QA Steps

The following python script can be used to verify both the bug in the current version as well as the fix implemented:

import asyncio
from juju.model import Model

bundle_file = "./bundle.yaml"

bundle = """
name: sample-bundle

series: jammy

machines:
  "0":
    constraints: instance-type=type1

applications:
  swift-storage:
    charm: swift-storage
    channel: yoga/stable
    num_units: 1
    to:
      - "0"
    storage:
      block-devices: cinder,1,5G
"""

async def main():
    with open(bundle_file, "w") as f:
        f.write(bundle)

    model = Model()
    await model.connect()
    await model.deploy(bundle_file)

asyncio.run(main())

All CI tests need to pass.

Notes & Discussion

I wasn't able to add a regression test for this fix since the juju-qa-test charm does not support the addition of storage devices. With that said, I'm open to suggestions on how to test this behavior to make sure it's correct and that it stays that way on future changes.

[jujubot]

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

[jujubot]

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

[kwmonroe]

this bit me as well on juju==3.4.0.0. confirmed this PR fixes this on that release.

[cderici]

Thanks for the change @luissimas! Code looks good 👍 Let's add the small bundle in your QA steps as an integration test, please use the tempfile so the file doesn't persist (e.g. with tempfile.NamedTemporaryFile() as temp_file:), you can just write, flush, then temp_file.seek(0), before running the deploy inside the context.

Alternatively, I'd be ok with adding this among our test bundles, if that's gonna be easier.

[cderici]

@jack-w-shaw can you add the following cases in your queue for fixing CI:

In this run for this PR:

• test_relate_with_offer
• test_app_relation_destroy_block_until_done
• test_add_manual_machine_ssh and test_add_manual_machine_ssh_root, not sure why these started failing, something to do with the offer URL it looks like

(note to @luissimas, none of the CI failures seems to be related to your changes, so we can land this as soon as you add the regression test, thanks again for the change ❤️)

[luissimas]

@cderici I've added a integration test in 132f96c. I had to make some changes to the bundle to be able to deploy it on an LXD cloud, which I assume is the expected environment for the integration tests. With that said, I was able to reproduce both the bug and the fix using this bundle.

[cderici]

/build

[cderici]

/merge