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

fix: move parse_storage_constraints to the bundle deployer #1105

Merged
merged 5 commits into from
Oct 17, 2024

Conversation

Aflynn50
Copy link
Contributor

@Aflynn50 Aflynn50 commented Sep 20, 2024

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

@skourta
Copy link

skourta commented Oct 11, 2024

+1. This is breaking our test in OpenSearch. The issue can also be solved by explicitly converting the constraints to their str representation in the parse_storage_constraint function.

@benhoyt benhoyt changed the title WIP fix: move parse_storage_constraints to the bunlde deployer WIP fix: move parse_storage_constraints to the bundle deployer Oct 13, 2024
@james-garner-canonical james-garner-canonical changed the title WIP fix: move parse_storage_constraints to the bundle deployer fix: move parse_storage_constraints to the bundle deployer Oct 15, 2024
Copy link
Contributor

@dimaqq dimaqq left a 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.

Copy link
Member

@wallyworld wallyworld left a 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.

juju/model.py Show resolved Hide resolved
tests/unit/test_bundle.py Show resolved Hide resolved
jujubot added a commit that referenced this pull request Oct 16, 2024
…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).
Aflynn50 and others added 4 commits October 17, 2024 10:18
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.
@james-garner-canonical
Copy link
Contributor

/merge

@jujubot jujubot merged commit 0e8c45f into juju:main Oct 17, 2024
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants