Use venvs with poetry in Dockerfile#2268
Merged
Merged
Conversation
ChrisLovering
requested review from
Den4200,
MarkKoz,
SebastiaanZ and
jb3
as code owners
HassanAbouelela
previously approved these changes
Sep 1, 2022
D0rs4n
previously approved these changes
Sep 1, 2022
ChrisLovering
added
the
review: do not merge
ChrisLovering
force-pushed
the
pin-poetry-to-1.1.X
branch
from
46a1760 to
234b426
Compare
ChrisLovering
removed
the
review: do not merge
ChrisLovering
changed the title
shenanigansd
approved these changes
Sep 4, 2022
Contributor
shenanigansd
left a comment
shenanigansd
left a comment
There was a problem hiding this comment.
Docker image builds successfully
wookie184
approved these changes
Sep 10, 2022
Contributor
wookie184
left a comment
wookie184
left a comment
There was a problem hiding this comment.
Can't say I know fully what's happening but the checks on this PR aren't failing, which is better than what we have on main :P
D0rs4n
approved these changes
Sep 10, 2022
wookie184
added
t: bug
soupglasses
reviewed
Sep 10, 2022
ChrisLovering
force-pushed
the
pin-poetry-to-1.1.X
branch
2 times, most recently
from
2ece958 to
f413776
Compare
This is required due to a regression in poetry, see HassanAbouelela/actions#7
ChrisLovering
force-pushed
the
pin-poetry-to-1.1.X
branch
from
f413776 to
459a103
Compare
ChrisLovering
force-pushed
the
pin-poetry-to-1.1.X
branch
2 times, most recently
from
99c742a to
c2f3736
Compare
HassanAbouelela
suggested changes
Sep 12, 2022
| dockerfile: Dockerfile | ||
| volumes: | ||
| - .:/bot:ro | ||
| - ./bot:/bot/bot:ro |
Contributor
There was a problem hiding this comment.
Why was this change made, don’t we still want the other files and directories such as tests and configs?
Member
Author
There was a problem hiding this comment.
This was made because I was putting the venv inside APP_DIR, causing venv on host to overwrite it with this volume.
It's not needed now though.
Member
Author
There was a problem hiding this comment.
Dropped the commit that added this
ChrisLovering
force-pushed
the
pin-poetry-to-1.1.X
branch
from
c2f3736 to
0b495e7
Compare
Instead let poetry install the venv for the project in the right place, leading to a more 'traditional' poetry setup.
ChrisLovering
force-pushed
the
pin-poetry-to-1.1.X
branch
from
32661bc to
03a3a03
Compare
HassanAbouelela
approved these changes
Sep 12, 2022
HassanAbouelela
added a commit
that referenced
this pull request
Sep 17, 2022
Poetry 1.2 introduced a regression which broke pip `--user` installs. These types of install were the main way we did installations in docker and CI, as they made it much more convenient to control the location, availability, and caching of packages. Poetry's team does not recognize this as a supported use case, so major changes were required to get everything working again. Most of the changes were consolidated into chrislovering/python-poetry-base for docker, and HassanAbouelela/setup-python for CI. This is a follow-up to #2268 and #2276. Signed-off-by: Hassan Abouelela <hassan@hassanamr.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is due to a regression in poetry 1.2.0, see HassanAbouelela/actions#7
Without this change, CI is currently failing to install deps, due to us using user-based installs.