-
Notifications
You must be signed in to change notification settings - Fork 100
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: move parse_storage_constraints to the bundle deployer #1105
Conversation
+1. This is breaking our test in OpenSearch. The issue can also be solved by explicitly converting the constraints to their |
9defd9d
to
518657f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I'd appreciate a review from someone on the Juju team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good but I'd like to see a little extra validation and test coverage.
…pe-hint-constraints #1159 #### Description While looking into #1105, I found it useful to type annotate `juju/constraints.py` while figuring out how things work. #### QA Steps There should be no runtime changes in behaviour. Unit and integration tests should continue to pass, especially `tests/unit/test_constraints.py`. #### Notes & Discussion I also broke the longer regexes down across multiple lines and added comments to help me understand them. I don't mind cutting this PR back to use more minimal type annotations, etc if that's deemed more appropriate in review (e.g. Dict[str, Union[int, float, bool]] instead of the custom TypedDicts).
It seems a previous change for allow bundle storage constraints was done at the wrong level and affected regualr storage constraints as well. This fix moves the previous change to only affect bunlde storage constraints.
4e45bc9
to
6ad2da9
Compare
6ad2da9
to
04f9a1d
Compare
/merge |
It seems a previous change for allow bundle storage constraints was done at the wrong level and affected regular storage constraints as well. This fix moves the previous change to only affect bundle storage constraints.
QA Steps
TODO