Ensure correct dependencies are installed for lowerbounds and cloud storage tests #822
Conversation
| @@ -17,7 +19,7 @@ RUN pip3 install | |||
| {{ " " }}{{ item.source }} | |||
| {%- if item.name == "pulpcore" -%} | |||
| {%- if s3_test | default(false) -%} | |||
| [s3] | |||
| [s3] git+https://github.com/fabricio-aguiar/botocore.git@fix-100-continue | |||
There was a problem hiding this comment.
I wonder if we needed a rebased version of this fix. Also how do we handle this in production?
Do we need to vendor botocore?
There was a problem hiding this comment.
It only affects MinIO, so if users are using that as their backend they will run into a timeout issue after a 0-byte file is uploaded. Luckily with Pulp's artifact system this will only occur once as the second time a 0-byte file is downloaded/uploaded the artifact will be re-used. Also, looking at the Deb tests that trigger this timeout it doesn't actually fail the sync since we have retries on downloads after the timeout. It would probably be good to use a rebased version of this fix.
Also, I've come to realize this bit of code for backend specific installs only works in the pulpcore repository since the plugins list will usually only have the plugin it's testing.
Second thing I've realized is that our lowerbounds test runner is no longer doing anything with the addition of the build step. We modify the requirements.txt after the build step has already ran. We probably need to use the output of .ci/scripts/calc_deps_lowerbounds.py as a constraint file for the pip install. I'll see if I can modify this PR to include that.
There was a problem hiding this comment.
The build step only builds a python package of the plugin. I do not see how this affects the lowerbounds test. But I may need a fresh look tomorrow.
There was a problem hiding this comment.
Builds include the required dependencies list in them. This is where pip gets the info on how to install dependencies.
There was a problem hiding this comment.
I think, i get what you say. We only pass the built whl to pip inside the container forgetting about the just adapted requirements.txt. I'll find a solution for that.
edit: Press reload first, next time!
gerrod3
changed the title
|
I realized my earlier change to ensure django-storage swas installing from the pulpcore requirements was actually only occurring in the pulpcore repository. It just happens that we install django-storages[boto3,azure] in the base pulp image (https://github.com/pulp/pulp-oci-images/blob/latest/images/Containerfile.core.base#L79), so we didn't see any errors in the plugin repositories. Also, lowerbounds was no longer installing from the recalculated requirements.txt since we switched to the build step, so I fixed that by passing it in as a constraint file. pulpcore test pr: pulp/pulpcore#4849 |
| {% if item.source.startswith("./") or item.ci_requirements | default(false) %} | ||
| {% if item.source.startswith("./") or item.ci_requirements or item.lowerbounds | default(false) %} |
There was a problem hiding this comment.
Let me think, item.source.startswith("./") should not happen anymore, right? And the rest are rather shallow conditions. Why not just skip the if altogether and add them all to the container?
| @@ -62,7 +62,6 @@ fi | |||
|
|
|||
| if [[ "$TEST" = "lowerbounds" ]]; then | |||
| python3 .ci/scripts/calc_deps_lowerbounds.py > lowerbounds_requirements.txt | |||
| mv lowerbounds_requirements.txt requirements.txt | |||
There was a problem hiding this comment.
I love this part of the change!
Shall we name the file lowerbounds_contstraints.txt instead?
| {%- if test_s3 %} | ||
| if [ "$TEST" = "s3" ]; then | ||
| cat >> vars/main.yaml << VARSYAML | ||
| pulpcore: "${PULPCORE_PREFIX}[s3]" |
There was a problem hiding this comment.
This is really hard to grasp.
Can we please not have the shell variable PULPCORE_PREFIX. Also can we add this suffix to source in case of pulpcore and maybe even for the plugins?
You can set jinja2 variables in the template if that helps any.
There was a problem hiding this comment.
I refactored this bit so that the generated install.sh is easier to read, see https://github.com/pulp/pulp_python/pull/638/files#diff-02059c243ab1ae5faa6a95ec965f0b351e0f6c96e5452c90881b6b7e3ee11618R40-R46
|
|
||
| RUN pip3 install | ||
| {%- if s3_test | default(false) -%} | ||
| {{ " " }}git+https://github.com/fabricio-aguiar/botocore.git@fix-100-continue | ||
| {%- endif -%} | ||
| {%- for item in plugins -%} | ||
| {%- if item.name == "pulp-certguard" -%} | ||
| {{ " " }}python-dateutil rhsm |
There was a problem hiding this comment.
Can we move that into certguards ci-requirements?
There was a problem hiding this comment.
Yes, going to require a separate PR though. Interestingly rhsm is installed in the base image (https://github.com/pulp/pulp-oci-images/blob/latest/images/Containerfile.core.base#L75), but not python-dateutil.
There was a problem hiding this comment.
Ooof 🤦.
Anyway:
pulp/pulp-certguard#331
…torage tests +Add back patched install for botocore for S3 testing Using MinIO for our S3 test backend requires us to use this patch to avoid timeouts when uploading 0-byte files. [noissue]
Using MinIO for our S3 test backend requires us to use this patch to avoid timeouts when uploading 0-byte files.
[noissue]