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

unify parse_storage_constraint or client.Constraints during deploy #1079

Closed

Conversation

addyess
Copy link
Contributor

@addyess addyess commented Jul 26, 2024

Description

Fixes #1075 by collapsing both storage constraint deviations in model._deploy

QA Steps

tox -e integration -- tests/integration/test_model.py -k test_deploy_with_storage

All CI tests need to pass.

Notes & Discussion

Perhaps it's not the best change, feel free to scrap and adjust better.

@jujubot
Copy link
Contributor

jujubot commented Jul 26, 2024

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

1 similar comment
@jujubot
Copy link
Contributor

jujubot commented Jul 26, 2024

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

@addyess addyess force-pushed the issues/1075/storage-constraint-on-deploy branch from f931900 to 183979a Compare July 26, 2024 18:54
@addyess addyess force-pushed the issues/1075/storage-constraint-on-deploy branch 2 times, most recently from 0d08092 to 3c4bded Compare August 1, 2024 15:49
@addyess addyess force-pushed the issues/1075/storage-constraint-on-deploy branch from 3c4bded to 3eb4cd3 Compare August 23, 2024 15:18
@addyess
Copy link
Contributor Author

addyess commented Aug 23, 2024

@cderici do you think this is a PR that could be reviewed?

@dimaqq
Copy link
Contributor

dimaqq commented Nov 19, 2024

This PR needs to be rebased since #1186 has been merged.

@addyess
Copy link
Contributor Author

addyess commented Nov 20, 2024

@dimaqq any chance this PR can get a look now?

@dimaqq
Copy link
Contributor

dimaqq commented Nov 21, 2024

I've asked on Matrix for a review.

Meanwhile, please run pre-commit on your change.

@dimaqq
Copy link
Contributor

dimaqq commented Nov 21, 2024

I had a cursory look, the changes are in the _deploy() function.

It's used only twice, in Model's deploy() and bundle's deploy().

Model's higher level function already coerces this argument, but maybe not correctly? that is it allows dicts but not strings.

I feel that this change needs to be refactored at the very least.

@addyess
Copy link
Contributor Author

addyess commented Nov 21, 2024

⚡ pre-commit run --files $(echo juju/client/_[cd]*.py) 
check for added large files..............................................Passed
check python ast.........................................................Passed
check for case conflicts.................................................Passed
check that executables have shebangs.................(no files to check)Skipped
check that scripts with shebangs are executable..........................Passed
check for merge conflicts................................................Passed
check for broken symlinks............................(no files to check)Skipped
check json...........................................(no files to check)Skipped
check yaml...........................................(no files to check)Skipped
check toml...........................................(no files to check)Skipped
mixed line ending........................................................Passed
fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
detect private key.......................................................Passed
ruff.....................................................................Passed
ruff-format..............................................................Passed
codespell................................................................Passed

@addyess
Copy link
Contributor Author

addyess commented Nov 21, 2024

I wonder if this was actually already fixed by @james-garner-canonical in #1105 in which case we could close this PR and the associated issue #1075

@james-garner-canonical
Copy link
Contributor

james-garner-canonical commented Nov 22, 2024

Hi @addyess , it's plausible that #1105 did indeed fix #1075. It sounds like #1105 was fixing an error introduced in #1053, which was indeed released in 3.5.2.0 -- unfortunately #1105 wasn't linked to #1075 or your PR. I'll have to take a deeper look on Monday. Hopefully we can do a 3.5.2.2 release to get this fix to you.

@james-garner-canonical
Copy link
Contributor

Should now be fully resolved in main by

Thanks for your help with this, @adyess

Release coming soon!

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.

Fail to deploy charm with storage parameter
4 participants