-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
Older PG compat #1306
Older PG compat #1306
Conversation
Hi @Xowap. Thanks a lot for fixing this! We should also test older Postgresql versions during CI. I already added a PR draft for that (#1307). If you like, add the changes in the |
Makes much sense, adding it |
LGTM. @ewjoachim Ok for you, too? |
.github/workflows/ci.yml
Outdated
@@ -61,6 +61,46 @@ jobs: | |||
path: .coverage.${{ matrix.python-version }} | |||
include-hidden-files: true | |||
|
|||
older-db-versions: |
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'm a bit weary of adding so many tests. What about changing the existing python matrix so that at the same time as we test old python versions, we test old postgres too ?
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.
My first version exactly did this, but I did not like it. I don't want to run coverage or upload artifacts. Of course, one can do this conditionally, but the job becomes more complicated. Also, the matrix itself becomes more complex as we want to test every Python version, but not every Python version against every database version. So, overall, I find a separate job more clear.
But I can implement it in my other PR (#1307) how it would look like.
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.
So it would look something like this then. Do you think it's better? Or do you have something else in mind?
The --cov
quite matters (running the whole test suite on my Laptop 47s vs 55s).
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.
@Xowap I understood ewjoachim wrong, but his suggestion makes total sense now. Would you mind adapting the CI code to something like this: https://github.com/procrastinate-org/procrastinate/pull/1307/files
Sorry for the extra work and that I didn't think of it myself 🤦.
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.
@Xowap I had some time left and quickly changed it 🙂.
@Xowap Thanks a lot for the contribution! I will create a new fix release later. |
Thanks for making contributing a nice experience! Waiting eagerly to revert my |
It was a pleasure to welcome your first Procrastinate contribution :) See you around ! |
Closes #1305
Successful PR Checklist:
PR label(s):