[RFC] Revisiting our CI setup: GH Actions, RTD

[sirosen]

In #687 , we started to discuss our more general setup in terms of Azure Pipelines vs GitHub Actions (GH Actions).
A couple of us support the switch to GH Actions, and there's no strong objection to it. I'd like to try that out.

The setup I want to try would be something like a reusable workflow to run tox, in the repo, and then a job named build which runs the tox workflow over a matrix of python version, os, and tox env names. Now that reusable workflows supports using a local file, we can develop something in webargs until we like the result. If we then want to move it to a separate project like marshmallow-code/reusable-github-workflows, we can discuss and/or do that.

There are two notable jobs is one job in Azure Pipelines which I don't think belongs in the move to GH Actions:

• tox -e docs
• pypi publish (done in Replace Azure Pipelines with GitHub Actions #690)

For the docs, ReadTheDocs (RTD) offers a relatively new feature to build on PRs; I recall we discussed it when it was still in beta. I like this primarily because it's driven by the .readthedocs.yml in the repo, so we don't run into trouble where our tox -e docs run in CI differs from what happens in RTD. It also means that if we have a PR to update .readthedocs.yml config, it will be tested in the PR build.
All we have to do is enable this feature in the readthedocs.org admin pane and make sure that the RTD webhook is set to send Pull Request events. A trial PR could demonstrate that this works -- e.g. by updating the python version used for RTD.

For pypi publishing, I think we can leave this in Azure for the initial move to GitHub Actions. It should be possible to setup a tag->publish reusable workflow, but I want to get more "basic" CI worked out before taking a crack at this. (done in #690)

---

With #690 now merged, we have a short "remaining" TODO list:

• Turn on ReadTheDocs PR builds (Settings > Advanced > PR builds)
• Remove testpypi releasing once we've tested that it works
• evaluate reusable workflows so that we can reduce duplication in config across repos

[lafrech]

Here's the GHA setup I use in flask-smorest.

https://github.com/marshmallow-code/flask-smorest/blob/master/.github/workflows/build-release.yml

It uses a PYPI_API_TOKEN I defined in Settings -> Security -> Secrets -> Actions.

[sirosen]

That's a good approach!

I've been somewhat disappointed with the publicly available actions for publishing to pypi in the past. They just don't seem to offer very much in exchange for another dependency in the critical path to publishing.
Writing it like this seems better. It's extremely simple and I don't need to jump out to a github repo and start reading an action.yml file to try to figure out what it does.

[sirosen]

From #687 , there was a question of how to make the release step depend on linting. In the flask-smorest build-release workflow, this is done with needs. I think that the current azure-pipelines setup doesn't enforce that other jobs pass before the release job runs. So we'd need to change the release process a little anyway.

I think the simplest thing is to have a near-verbatim copy of the flask-smorest release workflow. So there's some minor duplicate linting, but we make sure a release passes checks.

[lafrech]

On a related topic, I've been using pip-tools recently (https://github.com/BEMServer/bemserver-core, https://github.com/BEMServer/bemserver-api), inspired by Pallets repositories and API Flask [1]. I like the fact that the build can be reproducible with all dependencies pinned recursively.

I did it a bit differently, with a pre-commit action ensuring pip-compile is run when needed.

I find it nice, although not perfect. Not perfect for Pallets either, apparently: pallets/werkzeug#2334

What I dislike is the fact that everything is pinned but pre-commit and tox. Maybe it's not that bad. To actually pin them, I'd need to add dedicated requirement files. Or add them to dev.in and install dev.txt in GHA and tox.ini, thus installing unneeded stuff (no need to install pre-commit in GHA tox action and no need to install tox inside tox task). I decided to just let it go, like they do in Pallets. After all, those are just tools, not library imported in my code. Maybe I'm overthinking it.

[1] When it comes to CI, marshmallow-code and Pallets are my sources of inspiration.

[lafrech]

Also related, we're facing CI limitations in apispec so I'm probably going to move to GHA as soon as I get the time.

marshmallow-code/apispec#747

[sirosen]

I've put in a PR for this, with most of what I want in place.
I'm going to see about doing a test version of a publishing build which pushes to test-pypi, but intended as follow-up work.

On a related topic, I've been using pip-tools recently (https://github.com/BEMServer/bemserver-core, https://github.com/BEMServer/bemserver-api), inspired by Pallets repositories and API Flask [1]. I like the fact that the build can be reproducible with all dependencies pinned recursively.

I have used pip-tools to deploy applications in the past -- and liked it -- but never as part of working on a library. I've also had good experiences using poetry, FWIW. I'll have to look again at what's being done in pallets to understand how pip-tools it's being used.

When it comes to CI, marshmallow-code and Pallets are my sources of inspiration.

💯 ! I agree! I also tend to look at what the pypa and psf repos have, as they often seem to do sophisticated and interesting things.

What I dislike is the fact that everything is pinned but pre-commit and tox. ... those are just tools, not library imported in my code. Maybe I'm overthinking it.

I think it's good to think about. At this point, I would actually recommend against pinning things that don't fit neatly into the common pattern of "dev dependencies".

I used to try to pin these sorts of things aggressively in my workflows, and I've stopped after finding that I wasn't getting very much out of it other than busywork. e.g. I used to pin the version of twine in one of my projects, but then realized that we were just blindly bumping to the next version anyway.

We might need to re-assess after the tox rewrite is done and tox v4 is released, but I'm hoping that all of our configs will be supported and that we'll not need to make any changes.

[lafrech]

I used to pin the version of twine in one of my projects, but then realized that we were just blindly bumping to the next version anyway.

Yeah, exactly my point. Even if it breaks with tox 4, it should be obvious what the problem is and we'll fix it then. No need to add extra work for each minor and patch version.

What can be weird with the setup in Pallets that I copied (IIUC) is that stuff in requirements/dev.in is pinned, but the versions of tox and pre-commit are actually not the version used during the test.

[davidism]

the versions of tox and pre-commit are actually not the version used during the test

We now use pre-commit.ci which uses a specific pre-commit image, but pre-commit has other guarantees about compatibility and pinning hooks.

Now that you mention it, tox not being pinned was an oversight. With pip-compile-multi, it's absolutely fine to split up into lots of small requirements files, like one for a pre-commit env (again, check out pre-commit.ci), one for tox, etc. Although I guess it turns out it wasn't a big deal that it was unpinned, it hasn't caused any issues since I started doing all this.

The nice part about having all dev dependencies pinned is that it's much easier to ensure new contributors have a known-good environment. This was important at conference sprints, where we'd need to get lots of people set up quickly and not run into weird version differences.

Not perfect for Pallets either

Just to be clear, we're fine with pip-tools, pip-compile-multi is a wrapper around it that automates updating lots of requirements files. What I was unhappy with was Dependabot, which was noisy and also only worked specifically with the output of plain pip-compile, not the slightly different output of pip-compile-multi.

[sirosen]

I think we just forgot about this issue. Closing as complete (doing some open issue review right now).