Add Python 3.10 and 3.12 support #142
Merged
+28
−8
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jd
previously approved these changes
Jul 31, 2024
jd
previously approved these changes
Aug 1, 2024
|
Hi @jd, thanks for reviewing my PR. I got similar errors with the unit tests when running them locally, I thought they might be environment related so I wanted to run them on GitHub Actions just to be sure. Any potential insight as to what might be causing it? I'll troubleshoot this tomorrow morning, and see if I can figure out the cause. |
|
No clue, but you can probably debug this locally then :) |
|
(Note it might not be related to your PR) |
|
Running the tests with debug logging locally, I get this error from Gnocchi API.
Indeed, the EDIT: While this is indeed an issue, it doesn't appear to be the cause of the test failures. I'm unsure of the cause of the above error, as this shouldn't be caused the deprecated use of |
|
Here are the logs for the aggregates query that failed to find the metric. I'm not exactly sure how this works (getting a bit out of my depth here as I'm not that familiar with Gnocchi yet), but it looks like it might be a bug with gnocchixyz/gnocchi#1307 (the last major work with this code). |
|
Had to make a bunch of small modifications to replicate the older toolchain the tests used to run in (using OpenStack 2023.2 upper constraints), but I bisected and confirmed that gnocchixyz/gnocchi#1307 is the cause of the failed test. |
|
Submitted gnocchixyz/gnocchi#1392 to track the bug on the main Gnocchi repository. |
|
I have worked around the behaviour change by adapting the test so that it only uses timestamps generated after the metrics have been added to Gnocchi. Not sure if this is what we want to do (or if we want to fix the underlying bug), but it will get the test to pass. |
|
Looking at the failures for the latest test, seems like it's just a transient error and it'll probably pass if run again. (I got a lot of "server closed the connection" errors running locally, not sure what the cause is (might be pifpaf-related?), but running it again fixed it). |
tobias-urdin
previously requested changes
Aug 14, 2024
There was a problem hiding this comment.
thanks for looking into this, definitely something we want fix support for but i dont think changing the tests is the correct thing here instead resolving gnocchixyz/gnocchi#1392 and getting the ci working again with something like gnocchixyz/gnocchi#1395 is the correct way here
gnocchiclient/utils.py
Outdated
| :return: ``True`` if the value is a truthy value, otherwise ``False``. | ||
| :rtype: bool | ||
| """ | ||
| val = val.lower() |
There was a problem hiding this comment.
perhaps we do explicitly val = str(val).lower() to ensure we get a string
There was a problem hiding this comment.
Thanks for the comment, updated with your suggestion.
Fix python-gnocchiclient on Python 3.12 by replacing usage of distutils.util.strtobool with a new function in gnocchiclient.utils that does the same thing (except returning an actual boolean instead of an integer). Added Python 3.12 to the package Trove classifiers and tox.ini. Also added Python 3.10 to the package Trove classifiers and tox.ini, because it is generally an anti-pattern for a package to advertise Python 3.8-3.9 and Python 3.11+ support without also supporting Python 3.10. Fix pep8 checks by suppressing the new A005 rule.
mergify
bot
dismissed
tobias-urdin’s stale review
Pull request has been modified.
Hi Tobias, thank you for looking into the issues I've been having with the Gnocchi test suite. I've reverted the patch I made for the functional test, so that this can be merged once the fixes to Gnocchi have been made. Regarding the last CI failure for this PR, I forgot that I also needed gnocchixyz/gnocchi#1393 applied to have it pass locally. That's probably why the CI is not passing. |
|
Hi, could I please get the CI pipeline re-run? With the latest fixes to Gnocchi I'm confident it will pass this time. |
jd
reviewed
Aug 20, 2024
tobias-urdin
approved these changes
Aug 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix python-gnocchiclient on Python 3.12 by replacing usage of distutils.util.strtobool with a new function in gnocchiclient.utils that does the same thing (except returns an actual boolean instead of an integer).
Added Python 3.12 to the package Trove classifiers and tox.ini.
Also added Python 3.10 to the package Trove classifiers and tox.ini, because it is generally an anti-pattern for a package to advertise Python 3.8-3.9 and Python 3.11+ support without also supporting Python 3.10.
Fix pep8 checks by suppressing the new A005 rule.