-
Notifications
You must be signed in to change notification settings - Fork 89
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
Future code base components? (python 3.12 / pyproject.toml vs setup.py / poetry vs pip-pipenv-twine-... / docker improvements) #181
base: master
Are you sure you want to change the base?
Conversation
…mportlib.metadata and their backports (importlib_resources, importlib_metadata).
…of using the Pyproject.toml in favour of setup.py and setup.cfg.
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've taken a quick look and appreciate the changes overall. However, I have a few concerns about releasing with GitHub Actions that could use some improvement.
In summary:
- We should eliminate do_release.sh.
- The release version should be taken directly from Git tags, as it currently works this way.
- Running Docker as a non-root user is a significant advantage.
Unfortunately, I’ll have limited availability over the next two weeks.
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 file was not used anymore. We used release in GitHub actions with the help of the .VERSION file.
IMHO we can delete this file and see how we can get the version from a git tag with petry/toml?
RUN poetry install --without dev --no-root && rm -rf $POETRY_CACHE_DIR | ||
|
||
COPY --chown=withings-sync:withings-sync withings_sync ./withings_sync/ | ||
RUN poetry install --without dev |
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.
without a deeper understanding of poetry, can we compbine lines 20 -24 into two lines (copy all the required files and then install it)?
virtualenvs-create: false | ||
|
||
- name: Build & Package project 👷 | ||
run: poetry build | ||
|
||
- name: Publish distribution 📦 to PyPI | ||
if: startsWith(github.ref, 'refs/tags') |
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.
do we need poetry publish
here?
Also do wee need to set new environent variables in github action?
- uses: actions/checkout@master | ||
- name: Checkout code 🛒 | ||
uses: actions/checkout@v4 | ||
|
||
- name: Update version file ⬆️ | ||
uses: brettdorrans/[email protected] |
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.
if we can use the git tag as a source of version, we can get rid of this step and the .VERSION file.
Thx for taking the time already, I don't think we need to rush this, nothing is on fire atm. |
When working on the last PR I came across the statements that "The recommended way of using Setuptools has shifted in the direction of using the pyproject.toml in favour of setup.py and setup.cfg."
Combining this with "pkg_resources is deprecated" in python 3.12 and the docker wishlist in #133 I realized that we probably will have to introduce some breaking changes in the future.
Hence introducing this PR, primarily as a proposition of a possible way forwards. This introduces the following changes:
Again, this will introduce breaking changes, this PR is incomplete but I wanted to share this early to start a discussion/collaboration. If this is not the correct way forwards or not what you envisioned, just let me know.
todo: