-
-
Notifications
You must be signed in to change notification settings - Fork 42
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
Use latest versions of python libraries for testing #73
base: master
Are you sure you want to change the base?
Conversation
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.
Hi there!
I'm not sure if this is a good idea... I guess CI should try to mimic as close as possible what's usually in a production environment.
Of course, all production environments are different. For as much as I'd love Odoo to upgrade their dependencies more often, the fact is that the official set of supported ones is that outdated requirements.txt
they ship. You can check any recent build log in their runbot to know that.
Thus don't you think the conservative approach would be better?
Some comments also in code.
@@ -0,0 +1,5 @@ | |||
https://raw.githubusercontent.com/OCA/OCB/11.0/requirements.txt |
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 think this is required:
https://raw.githubusercontent.com/OCA/OCB/11.0/requirements.txt | |
-r https://raw.githubusercontent.com/OCA/OCB/11.0/requirements.txt |
@@ -0,0 +1,5 @@ | |||
https://raw.githubusercontent.com/OCA/OCB/12.0/requirements.txt |
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.
https://raw.githubusercontent.com/OCA/OCB/12.0/requirements.txt | |
-r https://raw.githubusercontent.com/OCA/OCB/12.0/requirements.txt |
@@ -0,0 +1,5 @@ | |||
https://raw.githubusercontent.com/OCA/OCB/13.0/requirements.txt |
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.
https://raw.githubusercontent.com/OCA/OCB/13.0/requirements.txt | |
-r https://raw.githubusercontent.com/OCA/OCB/13.0/requirements.txt |
@@ -0,0 +1,5 @@ | |||
https://raw.githubusercontent.com/OCA/OCB/14.0/requirements.txt |
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.
https://raw.githubusercontent.com/OCA/OCB/14.0/requirements.txt | |
-r https://raw.githubusercontent.com/OCA/OCB/14.0/requirements.txt |
|
||
# Install Odoo requirements. | ||
COPY requirements-${odoo_version}.txt /tmp/requirements.txt | ||
RUN pip install --no-cache-dir --no-binary psycopg2 -r /tmp/requirements.txt |
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.
This is already in the files, right?
RUN pip install --no-cache-dir --no-binary psycopg2 -r /tmp/requirements.txt | |
RUN pip install --no-cache-dir -r /tmp/requirements.txt |
Perhaps we could leverage the fact that we test twice (with odoo and OCB) to have one doing the conservative test and the other more bleeding edge. |
7bcd072
to
eb9f555
Compare
Odoo's requirements.txt pins old (sometimes very old) versions of python libraries. Their logic is to match what is available is some Debian versions they support.
There are downsides to testing with or relying on these versions:
With this PR, we instead rely on the latest versions, and we place upper bounds when there are known incompatibilities.
Another benefit of this approach is that we will detect early when there are incompatibilities with latest versions of these libraries. The corresponding downside is that it will create some noise when this happens, but it will be easy to update the requirements-*.txt here.
Another downside of this approach is that since we will test with last versions, developers may start relying on latest feature that are not compatible with the old versions.
It's a question of tradeoffs of course. In my view this PR will likely bring more benefits than downsides and is worth trying.