Skip to content
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

Improve test and release process #52

Merged
merged 21 commits into from
Aug 17, 2022
Merged

Improve test and release process #52

merged 21 commits into from
Aug 17, 2022

Conversation

guilhermef
Copy link
Member

@guilhermef guilhermef commented May 30, 2022

It's the same build environment used in libthumbor and thumbor-aws.

This PR will add:

  • Stalebot
  • Dependabot
  • CodeQL
  • release workflow
  • Linting on unittest workflow
  • Unittest workflow dependency cache

@marcelometal
Copy link
Member

Hi @guilhermef ,
Why re-migrate to poetry? PEP 517?

@guilhermef
Copy link
Member Author

It's mostly to be able to reuse the cached dependencies, and it makes it easier to pin dependencies.
The unittest workflow was also updated to benefit from it.

Copy link
Member

@heynemann heynemann left a comment

Choose a reason for hiding this comment

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

If noone opposes, I'm good with this. Just don't do it in thumbor's main repo. Other than that, it should be fine.

@marcelometal
Copy link
Member

IMHO, we can go without poetry, there is no real gain and we are adding another third-party dependency to the project

Copy link
Member

@scorphus scorphus left a comment

Choose a reason for hiding this comment

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

I also give poetry a strong 👎 We don't need it. And it's yet another dependency.

In fact, I think we should get rid of poetry entirely, thumbor-wise I mean. Yesterday I was debugging libthumbor and had to go over this loop:

  • edit/save
  • poetry build
  • ...wait...
  • pip install dist/libthumbor-x.y.z.tar.gz
  • ...wait...
  • experiment
  • repeat

There's definitely no poetry in that, whatsoever. And, BTW, python-poetry/poetry#34 is a joke.

The other aspects of this PR are stellar.

@scorphus
Copy link
Member

scorphus commented Jun 1, 2022

Also, thumbor/libthumbor#52 (comment)... meh.

@guilhermef
Copy link
Member Author

@scorphus you don't need to install the library.
Normally, I use poetry run <bin>

@guilhermef
Copy link
Member Author

Poetry does offer GIthub actions to cache the build environment, it helps to speed up the build process and it pins the version across builds.

@scorphus
Copy link
Member

scorphus commented Jun 1, 2022

@scorphus you don't need to install the library. Normally, I use poetry run <bin>

There's also this. A whole collection of new commands and ideas. A few more slots dislodged in my brain. I don't think I want that. I already know how to work with Python and its ecosystem.

Poetry does offer GIthub actions to cache the build environment, it helps to speed up the build process and it pins the version across builds.

There are other less intrusive ways to achieve both these — arguably desirable — things.

@guilhermef
Copy link
Member Author

@scorphus you don't need to install the library. Normally, I use poetry run <bin>

There's also this. A whole collection of new commands and ideas. A few more slots dislodged in my brain. I don't think I want that. I already know how to work with Python and its ecosystem.

Normally I would introduce a make run, to avoid this independent of the tool we choose.

Poetry does offer GIthub actions to cache the build environment, it helps to speed up the build process and it pins the version across builds.

There are other less intrusive ways to achieve both these — arguably desirable — things.

I agree, but I couldn't find an easier one that I didn't have to maintain.

@guilhermef
Copy link
Member Author

@scorphus, my suggestion would be to keep poetry while we don't have a better solution for environment caching on builds, and I'll add more docs to avoid confusion on how to run the projects with poetry.

@scorphus
Copy link
Member

scorphus commented Jun 2, 2022

@guilhermef, what could other options be in your view?

[...] Just don't do it in thumbor's main repo. Other than that, it should be fine.

@heynemann, why you think we shouldn't do this in thumbor's main repo?

@guilhermef
Copy link
Member Author

@scorphus the other option would be copying exactly what's being done by poetry, but on our workflow.

@heynemann
Copy link
Member

The only reason we can't do poetry in thumbor is that when I did it screwed the C integration for thumbor. So let's leave thumbor with setup.py as the way it handles C modules is quite particular.

@scorphus
Copy link
Member

scorphus commented Jun 6, 2022

@marcelometal what are your thoughts? Apart from previous words...

Also, do we package remotecv? What's your experience with debian packages that use poetry?

@marcelometal
Copy link
Member

Hi @scorphus , sorry for the late reply...

Would be nice packaging remoteCV for Debian =)

IMHO, we should avoid to include third-party libs, because it increase the complexity...
The latest upstream version is not the same in Debian and should not have all the features we will use, for example.

Debian: poetry: 1.0.7
Upstream: poetry: 1.1.13

@guilhermef
Copy link
Member Author

Ok, then I'll remove poetry and the environment cache.

@guilhermef guilhermef changed the title Move to poetry Improve test and release process Jul 24, 2022
@guilhermef guilhermef requested a review from scorphus July 24, 2022 22:05
@guilhermef
Copy link
Member Author

@scorphus @marcelometal I've removed Poetry dependency.

Copy link
Member

@scorphus scorphus left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thanks, @guilhermef

Please consider a couple suggestions below.

.github/codeql/codeql-config.yml Outdated Show resolved Hide resolved
.github/workflows/unittest.yaml Outdated Show resolved Hide resolved
flake8 Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
guilhermef and others added 2 commits August 15, 2022 11:29
Co-authored-by: Pablo Aguiar <scorphus@gmail.com>
Co-authored-by: Pablo Aguiar <scorphus@gmail.com>
@codeclimate
Copy link

codeclimate bot commented Aug 15, 2022

Code Climate has analyzed commit 59b4b15 and detected 0 issues on this pull request.

View more on Code Climate.

@guilhermef guilhermef merged commit 1619f9b into master Aug 17, 2022
@guilhermef guilhermef deleted the move-to-poetry branch August 17, 2022 07:28
@scorphus
Copy link
Member

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.

None yet

4 participants