feat: unify handling of storage constraints in deploy #1213
Conversation
james-garner-canonical
force-pushed
the
2024-11/feat/unify-handling-of-storage-constraints-in-deploy
branch
3 times, most recently
from
174d142 to
771e4f4
Compare
This allows both bundle.AddApplicationChange and model.Model.deploy to
use either storage constraints in the form of {label: constraint} where
constraint can be either a juju style storage constraints string, or a
python dict with a 'count' entry and optionally 'pool' and 'size' entries.
james-garner-canonical
force-pushed
the
2024-11/feat/unify-handling-of-storage-constraints-in-deploy
branch
2 times, most recently
from
1401e6f to
54bda34
Compare
| devices="devices", | ||
| channel="channel", | ||
| charm_origin=ANY, | ||
| num_units="num_units", | ||
| ) | ||
|
|
||
| async def test_run_with_storage_variations(self): |
There was a problem hiding this comment.
This test moves to test_constraints.py and no longer involves deployment
| @@ -627,7 +632,8 @@ async def run(self, context): | |||
| # NB: this should really be handled by the controller when generating the | |||
| # bundle change plan, and this short-term workaround may be missing some | |||
| # aspects of the logic which the CLI client contains to handle edge cases. | |||
| if self.application in context.model.applications: | |||
| model = cast("Model", context.model) # pyright: ignore[reportUnknownMemberType] | |||
There was a problem hiding this comment.
This is just to take a step towards type checking for the model._deploy call site (and have my IDE be able to identify the call site!)
There was a problem hiding this comment.
It seems that the context argument is always an instance of BundleHandler, and typing the context and and a few fields in the BundleHandler would do just as well.
Then again, I'm wary of having any PR grow in scope until the entire library is typed, so I've created a separate ticket, #1214
Unit tests have been updated to reflect the fact that parsing of storage constraints no longer happens before calling Model._deploy. Due to the difficulty in mocking the _deploy internals, the bundle test of parsing many storage variations has been moved to test_parse_storage_constraints in test_constraints. Two integration tests have been added to test deployment with parsed and unparsed storage arguments respectively.
james-garner-canonical
force-pushed
the
2024-11/feat/unify-handling-of-storage-constraints-in-deploy
branch
from
54bda34 to
69aa052
Compare
|
Failing tests are now just |
dimaqq
changed the title
| @@ -627,7 +632,8 @@ async def run(self, context): | |||
| # NB: this should really be handled by the controller when generating the | |||
| # bundle change plan, and this short-term workaround may be missing some | |||
| # aspects of the logic which the CLI client contains to handle edge cases. | |||
| if self.application in context.model.applications: | |||
| model = cast("Model", context.model) # pyright: ignore[reportUnknownMemberType] | |||
There was a problem hiding this comment.
It seems that the context argument is always an instance of BundleHandler, and typing the context and and a few fields in the BundleHandler would do just as well.
Then again, I'm wary of having any PR grow in scope until the entire library is typed, so I've created a separate ticket, #1214
| if isinstance(storage_constraint, str): | ||
| parsed[label] = parse_storage_constraint(storage_constraint) | ||
| elif isinstance(storage_constraint, dict): # pyright: ignore[reportUnnecessaryIsInstance] | ||
| parsed[label] = storage_constraint | ||
| else: | ||
| raise ValueError( |
|
/merge |
Description
This PR unifies storage constraint parsing into a single method (
juju.constraints.parse_storage_constraints), which is called in a single place (juju.model.Model._deploy), allowing both bundle and model deployments to specify storage constraints using either the juju storage constraint directive format (e.g.{'label': 'ebs,100G'}) or pre-parsed dictionaries (e.g.{'label': {'count': 1, 'pool': 'ebs', 'size': 102400}).QA Steps
Unit tests have been updated to reflect the new parsing location. Integration tests have been added to verify that model deployment can request storage using either format. The existing bundle integration tests should continue to pass.
Notes & Discussion
This PR resolves the issues with storage constraint parsing identified in:
#1052 was initially addressed in #1053, which was included in the 3.5.2.0 release. This allowed bundle deployments (using
juju.bundle.AddApplicationChange.run) to correctly handle storage constraints specified as strings in the juju storage constraint directive format.Unfortunately, this erroneously required that model deployments (using
juju.model.Model.deploy) also use this string format, instead of the parsed dictionary format that was previously accepted. This was noticed in #1075 and initially fixed in #1105, which was merged intomainbut never released. This fix moved parsing of storage constraint strings to bundle deployment exclusively.Due to the interim period in which
3.5.2has (incorrectly) required model deployments to use the string format, I think the best fix at this point is to allow both bundle deployments and model deployments to use either format, and parse them into the parsed dictionary format in a single place injuju.model.Model._deploy(the private method called by both bundle and model deployments).After merging, let's look at getting these changes out in a
3.5.2.2bugfix release.