Skip to content

Conversation

@roznawsk
Copy link
Member

@roznawsk roznawsk commented Oct 1, 2023

No description provided.

@roznawsk roznawsk force-pushed the use-poetry branch 18 times, most recently from 7604abf to 8ca87ef Compare October 1, 2023 21:15
@roznawsk roznawsk marked this pull request as ready for review October 2, 2023 06:04
@roznawsk roznawsk requested review from blazpie and sgfn October 2, 2023 06:05
Comment on lines 18 to 24
name: Install deps
command: apt update && apt upgrade && apt install -y curl gcc libc-dev libffi-dev
- run:
name: Install poetry
command: "curl -sSL https://install.python-poetry.org | python3 - && \
echo 'export PATH=/root/.local/bin:${PATH}' >> \"$BASH_ENV\" && \
source \"$BASH_ENV\""
Copy link
Member

Choose a reason for hiding this comment

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

As per https://python-poetry.org/docs/#ci-recommendations, I think we should:

  • lock to a specific version of poetry
  • install it elsewhere so that we don't have to update PATH

Same in the docker-compose-test.yaml file

- network

test:
image: python:3.8-alpine3.18
Copy link

Choose a reason for hiding this comment

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

is there a good reason to use different image in "lint" (python:3.8.18-slim-bullseye) and in test (python:3.8-alpine3.18) jobs?

* Use cimg image.

* Remove unused
@roznawsk
Copy link
Member Author

roznawsk commented Oct 3, 2023

Regarding both @blazpie and @sgfn comments:

I greatly improved the testing by using cimg image, which is ubuntu based and comes with poetry already installed.
Now both CI lint job and docker compose use this image. The image has pinned poetry 1.6 version.

@roznawsk roznawsk requested review from blazpie and sgfn October 3, 2023 14:39
pyproject.toml Outdated
]
description = "Python server SDK for the Jellyfish media server"
version = "0.1.0"
description = "\"Python server SDK for the Jellyfish media server\""
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description = "\"Python server SDK for the Jellyfish media server\""
description = "Python server SDK for the Jellyfish media server"

Copy link

@blazpie blazpie left a comment

Choose a reason for hiding this comment

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

LGTM

@roznawsk roznawsk merged commit 201750f into main Oct 4, 2023
@roznawsk roznawsk deleted the use-poetry branch October 4, 2023 10:22
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.

4 participants