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

Bump certifi #1080

Closed
wants to merge 1 commit into from
Closed

Bump certifi #1080

wants to merge 1 commit into from

Conversation

ttung
Copy link
Collaborator

@ttung ttung commented Mar 12, 2019

It seems like Docker build can't deal with requests to install older versions of certifi other than is installed by travis by default.

@ttung ttung requested a review from joshmoore March 12, 2019 20:15
@ttung
Copy link
Collaborator Author

ttung commented Mar 12, 2019

@joshmoore any idea why this is necessary?

here's the log from a failed run.

@joshmoore
Copy link
Member

any idea why this is necessary?

This last time I looked into this, I think I got dug long enough to find out that "conda and pip don't work flawlessly together". (cF. pypa/pip#5247)

This is where the --ignore-installed in #713 came from. What's changed here is that we now have a call to pip in environment.yml. I've had some success with adding flags to environment.yml files. I'd suggest trying:

  - pip:
    - --ignore-installed
    - ...

Another option would be to try to force conda use the version of certifi that pip is expecting.


NB: something I found while searching around that might be worth a second look -- https://picky-conda.readthedocs.io/en/latest/

@codecov-io
Copy link

codecov-io commented Mar 12, 2019

Codecov Report

Merging #1080 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1080   +/-   ##
=======================================
  Coverage   90.81%   90.81%           
=======================================
  Files         164      164           
  Lines        6151     6151           
=======================================
  Hits         5586     5586           
  Misses        565      565

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29012fc...4d8bf68. Read the comment docs.

@ttung
Copy link
Collaborator Author

ttung commented Mar 12, 2019

Looks like flags in the pip section of environment.yml is not supported.

further confirmed with https://travis-ci.com/spacetx/starfish/builds/104144068

It seems like Docker build can't deal with requests to install older versions of certifi other than is installed by travis by default.
@ttung ttung force-pushed the tonytung-certifi branch from 55fa306 to 4d8bf68 Compare March 14, 2019 05:54
Copy link
Member

@joshmoore joshmoore left a comment

Choose a reason for hiding this comment

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

No objections to the bump, but I'm not completely aware of possible impact to existing virtualenvs/condas. Would it be an option to filter this out of the strict list?

Another alternative I've found is to use a pip config file:

$ head *
==> config <==
[install]
ignore-installed = true

==> Dockerfile <==
FROM continuumio/miniconda3
RUN useradd -m starfish
USER starfish

# Set up the initial conda environment
COPY --chown=starfish:starfish environment.yml /src/environment.yml
COPY --chown=starfish:starfish REQUIREMENTS* /src/
COPY --chown=starfish:starfish config /src/
WORKDIR /src
ENV PIP_CONFIG_FILE=/src/config

==> environment.yml <==
name: test
channels:
- defaults
dependencies:
- python>=3.6
- pip:
  - -r REQUIREMENTS.txt

==> REQUIREMENTS.txt <==
certifi==2018.11.29

@ttung
Copy link
Collaborator Author

ttung commented Mar 15, 2019

I prefer this approach.

@ttung ttung mentioned this pull request Mar 15, 2019
@ttung
Copy link
Collaborator Author

ttung commented Mar 15, 2019

Closing in favor of #1085

@ttung ttung closed this Mar 15, 2019
@ttung ttung deleted the tonytung-certifi branch March 15, 2019 17:55
ttung pushed a commit that referenced this pull request Mar 15, 2019
This replaces #1080, and depends on #1084

Test plan: `make docker`
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.

3 participants