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

Revert commits that break session commits released with v2.2.0 #5851

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Dec 21, 2022

PR #5804 , which fixed #5802 and which was released with v2.2.0 introduced a way bigger bug that went unnoticed by the tests. Essentially, changes performed in a shell or by verdi commands are no longer committed and so not persisted to the database. I have been working on finding a fix without reverting, but it looks like this might take more time to do properly. I therefore propose to revert the relevant commits (while adding the test that fails due to #5802 , but skipping it for now) and make a v2.2.1 patch release. This is quite urgent because v2.2.0 is completely unusable at the moment.

@sphuber sphuber changed the base branch from main to release/2.2.1 December 21, 2022 19:33
@sphuber sphuber requested a review from ltalirz December 21, 2022 19:33
There currently is a bug in the `PsqlDosBackend` that when the session
is commited during `QueryBuilder.iterall`, for example by creating a new
ORM instance, an exception is raised. This bug was addressed in the
commit 98e902b, however, this caused
big problems that were not detected by the tests. Essentially, change
were no longer automatically committed and so changes performed in a
shell or by `verdi` commands would not get persisted to the database.

The introduced bug was released with `v2.2.0` but the responsible
commits have now been reverted. Here we add the test that demonstrates
the original bug that the reverted commits were trying to address as
described in aiidateam#5802. For
now the test is skipped since it fails, but once the bug is properly
fixed, without breaking everything else, it should be enabled.
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @sphuber , I agree with the urgency of releasing a fix and this way of proceeding

@giovannipizzi
Copy link
Member

Thanks @sphuber, I also agree to fix this urgently, while still prioritizing a solution for #5802. If you know people were affected by the bug, make sure to ping them that the bug will be back, thanks!

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.

4 participants