Boavizta / boaviztapi

🛠 Giving access to BOAVIZTA reference data and methodologies trough a RESTful API
GNU Affero General Public License v3.0
66 stars 23 forks source link

Release Github action fails #184

Closed demeringo closed 1 year ago

demeringo commented 1 year ago

Bug description

It seems that the Publish release (release.yml) github action does not work.

Looking at the action history, it seems that it never worked in the first place 😉, see result from past builds: https://github.com/Boavizta/boaviztapi/actions/workflows/release.yml

To Reproduce

Trigger the release action. It fails on the build stage, complaining about missing dependency.

I suspect that the command to build does not use the virtual env created at the previous step.

Current command executed is python3 setup.py sdist Maybe we should prefix it with a call to pipenv (something like pipenv run python setup.py sdist) ?

Expected behavior

Triggering the release action should perform the build and publish the image.

JSON OUTPUT

Additional context

⚠ I do not know much about python dependencies management... 🙃

demeringo commented 1 year ago

Or do we need to generate the requirements file before generating the sdist ?

https://pypi.org/project/pipenv-to-requirements/

demeringo commented 1 year ago

Other info https://medium.com/expedia-group-tech/simplifying-python-builds-74e76802444f

demeringo commented 1 year ago

Seems that the actual way to generate requirements is pipenv requirements > requirements.txt to run before the build phase.

demeringo commented 1 year ago

@da-ekchajzer why do we publish the package in the first place ? Is it really downloaded / used as a package by third party applications ?

I believe the main uses cases for the API are are either:

So the question is should we keep (and fix) the publication step or just remove it from the workflow ?

da-ekchajzer commented 1 year ago

@demeringo Indeed. I think that today the code is not intended to be used as a library. If it makes it simple to stop maintaining a pip package file, free to remove it from the pipeline !

demeringo commented 1 year ago

It seems that building the dist fails at some point.

I will try to remove this step, but it means also updating the DockerFile: It uses the dist to build the image. But this means that the docker build depends on a previous implicit step not documented in the docker file.

I would rather have a DockerFile that builds from source, using pipenv.

https://github.com/Boavizta/boaviztapi/blob/07b1f1a0366f60a847f8258aeef7dbc003c9f378/Dockerfile#L9

But I do not know python packaging very well, so please correct me if you think it is the wrong direction .

bamthomas commented 1 year ago

if I remember well when we did this it was to be able to use the package as a standalone library as well as a docker image and avoiding duplication.

Maybe we should prefix it with a call to pipenv (something like pipenv run python setup.py sdist) ?

yeah absolutely. Locally I can't build also, the dependencies are broken. Arf all the dependencies are * :(

demeringo commented 1 year ago

Yes it seems that mixing up pipenv and requirements.txt is far from prefect.

There is a option to recreate requirement.txt from pipenv (pipenv requirements). But I even doing so I did not manage to build properly on my laptop (but here maybe my particular python setup is involved...).

bamthomas commented 1 year ago

I'm making a PR to migrate the project with poetry (easier to maintain). I'm going to make the dependencies capped also.

demeringo commented 1 year ago

Thank you @bamthomas for the PR #191 👍👍👍

bamthomas commented 1 year ago

You're welcome. I'm not sure for now (publish process has not been tested yet) that it is solving this issue. Let me know if it isn't.

da-ekchajzer commented 1 year ago

Thank you very much @bamthomas. Works wonderfully !

https://github.com/Boavizta/boaviztapi/actions/runs/5175785662