-
Notifications
You must be signed in to change notification settings - Fork 10
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: remove minimum size requirements #97
Conversation
#78 Summary of changes: - Removed minimum size requirements from metadata.yaml
#78 Summary of changes: - Updated libraries
#78 Summary of changes: - Fixed tests to match update library and ops. - Modified tox.ini to ignore appropriate directories.
What is the behaviour when there is no minimum size set? Is it that there is no storage created, or that it just uses some juju default? I wonder if the best fix to #78 would be to remove the storage entirely. Is that what happens here? |
--skip {toxinidir}/build --skip {toxinidir}/lib --skip {toxinidir}/venv \ | ||
--skip {toxinidir}/.mypy_cache \ | ||
--skip {toxinidir}/icon.svg --skip *.json.tmpl | ||
codespell {toxinidir}/. --skip {toxinidir}/./.git --skip {toxinidir}/./.tox \ |
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.
Can you explain what issue you are having without this change?
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.
Those directories are not ignored by codespell
if /./
is not there. This is pretty much how it is done in other repos as well.
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.
I suggest splitting this and the library bump up into a separate PR so it is clear what fixes #78 and what is maintenance
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.
+1 on the above suggestion, please split this PR with those maintenance/CI fixes.
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.
Got it. Will do.
Storage is optional. No default minimum size is defined, as far as I can see. Charm is using |
If we cannot remove the storage entirely, what happens when we just omit the minimum size? What size volume is being created? I'm partly worried that, by not setting 10GB as the minimum, we're actually getting a larger volume |
Looks like without minimum set, PVC is 1G: |
#78
Summary of changes:
NOTE: Extra changes (in addition to one in
metadata.yaml
) are required to have update libraries and test operator framework.