Skip to content

Fix issue with None value attributes #1287

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

Merged

Conversation

rafaelweingartner
Copy link
Contributor

With Python 3, the sort function is not handling None values. Therefore, when we handle resources with values as None an exception happens. This patch fixes such situations.

@rafaelweingartner rafaelweingartner force-pushed the fix_gnocchi_issue_with_python3 branch 2 times, most recently from 39baffb to baea0a1 Compare January 30, 2023 12:15
@rafaelweingartner
Copy link
Contributor Author

@tobias-urdin this is also an important one. However, its tests will only pass after the following patches have been merged:

@tobias-urdin
Copy link
Contributor

Could this be reproduced with a test? I'm trying to understand what the change will do to the grouped data, will there be entries with an empty string? Should those be filtered out instead?

@rafaelweingartner
Copy link
Contributor Author

The code is not designed in a way that enables us to easily do a unit testing. Let me think about what we can do here.

@tobias-urdin
Copy link
Contributor

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Jan 31, 2023

⚠️ This pull request got rebased on behalf of a random user of the organization.
This behavior will change on the 1st February 2023, Mergify will pick the author of the pull request instead.

To get the future behavior now, you can configure bot_account options (e.g.: bot_account: { author } or update_bot_account: { author }.

Or you can create a dedicated github account for squash and rebase operations, and use it in different bot_account options.

@mergify
Copy link
Contributor

mergify bot commented Jan 31, 2023

rebase

✅ Branch has been successfully rebased

@chungg chungg force-pushed the fix_gnocchi_issue_with_python3 branch from baea0a1 to 97f04f8 Compare January 31, 2023 12:50
Version 2 of SQL Alchemy has been released, and it brings considerable changes to it [1]. The one that is affecting us right away is the philosophy of always using a transaction. That means, for every connection open with the database (DB), there is automatically a transaction opened. This, in turn, breaks the current code structure that we have, where we try to open a transaction every time we get a connection to write or read in the DB. The break happens because the code is trying to open a transaction inside an already existing one (the transaction opened by SQL Alchemy itself).

This whole situation only happens with SQL Alchemy >=2. Therefore, for now, the solution is to pin down its (SQL Alchemy) version until we have a final solution, which is going to have a considerable impact in the transaction management that we currently have.

[1] https://pypi.org/project/SQLAlchemy/2.0.0/
With Python 3, the ``sort`` function is not handling `None` values. Therefore, when we handle resources with values as ``None`` an exception happens. This patch fixes such situations.
@rafaelweingartner rafaelweingartner force-pushed the fix_gnocchi_issue_with_python3 branch from a7cc83a to d2fa86f Compare January 31, 2023 22:01
@rafaelweingartner
Copy link
Contributor Author

@tobias-urdin I added the unit tests for this one, as you suggested.

@tobias-urdin
Copy link
Contributor

Thanks @rafaelweingartner – let me know if you have anything else you want in master I will cut a 4.5 release sometime this week.

@mergify mergify bot merged commit 0221178 into gnocchixyz:master Feb 1, 2023
@rafaelweingartner
Copy link
Contributor Author

Thanks @rafaelweingartner – let me know if you have anything else you want in master I will cut a 4.5 release sometime this week.

With respect of Gnocchi server, I think everything is fine. Only the Gnocchi client where I Think there is that adjust in the handling of exceptions that we should discuss a little bit more, and maybe generate a new version of the client as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants