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

update conda store dockerfile to have prod target #621

Merged

Conversation

dcmcand
Copy link
Contributor

@dcmcand dcmcand commented Oct 12, 2023

Fixes #614
Part of #599

Description

updates Dockerfile for conda-store to have a prod target that does not install pip dependencies in editable mode, and a dev target that does.

This pull request:

  • Update Dockerfile as described above
  • Set github actions to use prod targets

Pull request checklist

  • Did you test this change locally?
  • Did you update the documentation (if required)?
  • Did you add/update relevant tests for this change (if required)?

@netlify
Copy link

netlify bot commented Oct 12, 2023

Deploy Preview for kaleidoscopic-dango-0cf31d canceled.

Name Link
🔨 Latest commit 4cc36d0
🔍 Latest deploy log https://app.netlify.com/sites/kaleidoscopic-dango-0cf31d/deploys/652d9a1e3918cb0008b01634


FROM base as prod
RUN cd /opt/conda-store && \
pip install .
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer installing directly from PyPA than from the repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@trallard Do you mean to install the conda-store package from pypi when packing the docker release?

right now, docker images are built and pushed on every merge to main, and on every release. Using PyPi would mean that we should split the workflow, so pushes to main used this repo (since it isn't pushed to PyPi), and releases installed from PyPi, which would add a dependency in the order that the jobs run.

I am fine with that if we want to do it that way, I just want to make sure we are on the same page here.

Copy link
Collaborator

@trallard trallard Oct 16, 2023

Choose a reason for hiding this comment

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

correct - I added more context in #621 (comment) since I got sidetracked 🧠

I do not think splitting the workflow would be too much of an inconvenience but will allow us to know exactly which tagged version of the library we have installed at a given time.

@trallard
Copy link
Collaborator

trallard commented Oct 16, 2023

While reviewing the PR I realised that the GH workflow logic actually needs some tuning too. Right now the triggers are:

on:
  push:
    branches:
      - "main"
  release:
    types: [created]

While this PR creates separate dev and prod Docker targets, the images are built more frequently than releases are made, and since conda-store is installed with pip install . this still does not ensure we are using an officially released version (which I'd prefer doing) so we might need to adapt to some logic where

  • dev is built on pushes to main and optionally when a new version has been released on pip or conda-forge
  • prod is built when a new version has been released on pip or conda-forge only

@dcmcand

@dcmcand
Copy link
Contributor Author

dcmcand commented Oct 16, 2023

While reviewing the PR I realised that the GH workflow logic actually needs some tuning too. Right now the triggers are:

on:
  push:
    branches:
      - "main"
  release:
    types: [created]

While this PR creates separate dev and prod Docker targets, the images are built more frequently than releases are made, and since conda-store is installed with pip install . this still does not ensure we are using an officially released version (which I'd prefer doing) so we might need to adapt to some logic where

* `dev` is built on pushes to main and optionally when a new version has been released on pip or conda-forge

* `prod` is built when a new version has been released on pip or conda-forge only

@dcmcand

Ok, no problem. I will make these changes and let you know when it is ready for 👀

@dcmcand
Copy link
Contributor Author

dcmcand commented Oct 16, 2023

@trallard the same applies to conda-store-server correct?

@trallard
Copy link
Collaborator

yes this should apply for both

@dcmcand
Copy link
Contributor Author

dcmcand commented Oct 16, 2023

@trallard This is ready for another look. We can't test the the full release workflow without cutting a release though. Can we cut a test release and then delete it from pypi after? Or are you comfortable with not testing that bit?

@dcmcand
Copy link
Contributor Author

dcmcand commented Oct 17, 2023

@trallard is this one good to go?

@dcmcand dcmcand requested a review from trallard October 17, 2023 19:19
@trallard
Copy link
Collaborator

🤔 I got confused since the changes here are also in #625, so it seems some of my changes will not be introduced here (e.g. timezone) but in the other PR which is fine by me so will mark this as merge ready.

As for the release - I will cut a release candidate today/tomorrow so that should work as a test

@trallard trallard merged commit 2753b44 into conda-incubator:main Oct 18, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done 💪🏾
Development

Successfully merging this pull request may close these issues.

[BUG] - conda-store image needs a release target and dev target in the Dockerfile
2 participants