-
Notifications
You must be signed in to change notification settings - Fork 26
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
switch dependency management and package installation to poetry #1304
Conversation
4b4efdb
to
53f3799
Compare
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.
Nice, I like it!
# Required by both public and admin apps | ||
python = "^3.9" | ||
connexion = {version = "*", extras = ["swagger-ui"]} | ||
dockerflow = "*" |
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 realize this is just copying what we previously had, so definitely not a consideration for this PR.. But we may want to consider using the compatible release operator here. Especially if we're going to let dependabot run loose with it. Otherwise we may end up pulling in major versions for multiple deps at a time which could make it difficult to track down bustage.
Then again, using compatible release might mean that we simply don't ever upgrade things which can also be problematic. So I'm not saying one way is wrong and the other isn't, more of some food for thought.
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.
At a high level, I think it's good allowing dependabot (or whatever) to attempt to upgrade packages unless we know they aren't compatible -- that way at least we get some feedback on the PR or PRs that are opened, and it makes it easy to pull in simple upgrades on a regular basis.
I agree that it's worth talking about this again soon though - it's been a long time since we've had an explicit conversation about how we manage this.
ENV WEB_CONCURRENCY=3 | ||
USER app | ||
|
||
CMD ["/app/docker.d/init.sh"] | ||
RUN poetry install --only main |
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 fine, just noting that if you want to avoid using poetry's virtualenvs and continue using the same Python installations as before (e.g to avoid poetry run
later on), you can use poetry export
to convert the poetry.lock
into a requirements.txt
and then pip install
that with whatever Python you want.
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.
And presumably this avoids a lot of the pip dependency resolver (and other) pitfalls, since pip is really just doing download + install (no resolution, etc.)?
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 looks great. Any concerns about using poetry for all releng projects?
I was thinking of this as a way to trial idea. I certainly have no concerns from the outset, and no objections if someone wants to similarly switch other projects. But I also think it's fine to stay the course with |
CMD ["/app/docker.d/init.sh"] | ||
RUN poetry install --only main | ||
|
||
CMD ["/app/docker.d/init.sh", "public"] |
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.
Could be a followup, but if the only difference between the admin and public Dockerfiles is the CMD then we could have just one of them, and set the command in docker-compose.yml?
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.
Yeah, we should do that (as we do in Balrog). I was trying to contain the scope/risk creep for now 😛
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 part looks like it will require coordination with a cloudops-infra
change, as we don't explicitly set the entrypoint nor command for our deployments. I filed #1310 for it.
I consider this an experiment to see if Poetry might be better than our current set-up. The main pros are:
pyproject.toml
, with the pinned versions ending up inpoetry.lock
. Bumping versions is trivial - we simply runpoetry lock
and it will regenerate the latter from the former.There are a couple of downsides:
I did my best to avoid changing anything else as part of this, but there was a couple of things I ended up doing: