Travis -> GitHub workflows

[GeorgianaElena]

No description provided.

[consideRatio]

@GeorgianaElena I've added a pypi_password in this repo as a github repo secret you can reference like @minrk has done in jupyterhub/oauthenticator! Thanks for your work on this!

[consideRatio]

Excellent work on this, thanks for your patience with my review comments @GeorgianaElena!

[GeorgianaElena]

Thanks for taking the time to review this @consideRatio ❤️ Your reviews are always an opportunity to learn about a lot.

I believe there's still a couple of things left to fix:

• if we add the tags condition, the build won't run on every push. It would be nice to be able to keep building on every push and only publish on tags with that match our regex. I'm thinking we have two options to do this:
  • separate build and push in two yml files
  • see if the condition startsWith of pypa/gh-action-pypi-publish@v1.4.1accepts regex (I didn't find docs about it, but I believe it's worth trying anyway).
• the build in the release job actually fails right now (see the run from yesterday: https://github.com/GeorgianaElena/traefik-proxy/runs/1391443808?check_suite_focus=true). It might have something to do with having setuptools listed in the pyproject.toml 🤷‍♀️ (though after reading https://snarky.ca/what-the-heck-is-pyproject-toml/ it doesn't have a lot of sense)

What do you think?

[consideRatio]

if we add the tags condition, the build won't run on every push.

Do you mean that if we add the tags condition, we won't run tests on pushes of tags not having a version constraint? I think that is fine.

I suggest to have the build package step as part of a test in the test suite, duplicated from the publishing workflow. If it is part of the publishing workflow that always run, I know that for me, it force me to inspect "did it REALLY publish, or did it just run the build package steps?" whenever I really want to know if it actually published during release.

My wishes in order of importance

1. To only trigger the publish workflow if we actually publish
2. To test the python package packaging
3. To not break DRY with regards to testing the python package packaging
4. To test the python package packaging on non-version like tags.

If 2-4 would add notable complexity or similar, I'd personally prefer to not bother about them. I have only felt the need to test the packaging part when I develop the CI for the packaging part, and even then, a test would not capture the validity of environment variables for deployment keys to PyPI etc.

[consideRatio]

Build error

I think it is caused doing python -m pip install --upgrade setuptools pip and then python -m build --sdist --wheel ., and it makes setuptools install twice or similar because of the pyproject.toml contain setuptools as a build-system dependency. I suggest to try removing the explicit installation of setuptools in a pip install step.

    - name: Install build package
      run: |
        python -m pip install --upgrade setuptools pip
        pip install build
        pip freeze
    - name: Build release
      run: |
        python -m build --sdist --wheel .
        ls -l dist

README badge

The README contain a badge pointing to travis still. Here is a badge in markdown for you to use, note it reference a specific workflow, and it contains the case-sensitive URL encoded name of the workflow which is currently Run tests.

[![GitHub Workflow Status](https://img.shields.io/github/workflow/status/jupyterhub/traefik-proxy/Run%20tests?logo=github)](https://github.com/jupyterhub/traefik-proxy/actions)

[GeorgianaElena]

Thanks for the feedback @consideRatio. I added a few commits in which:

• Added the new badge to readme 🎉
• Added the pkg build step to the python 3.8 test suite
• Removed completely the build requirements (wheel and setuptools) from pyproject.toml as they get installed as a build package requirement and otherwise it complains with:

  ERROR: Double requirement given: setuptools (from -r /tmp/build-reqs-y789h1lu.txt (line 3)) (already in setuptools>=40.8.0 (from -r /tmp/build-reqs-y789h1lu.txt (line 1)), name='setuptools')
  

New observation:
One proxy test (the first one that starts traefik) is flaky as it sometimes fails to wait for traefik to load it's config. I increased the start timeout for that proxy but still didn't manage to make it stable, so suggest will leave as it is (at least for this PR) and try different approaches later on to fix this.

What do you think @consideRatio ? Is there anything else I could do to improve this PR?

[consideRatio]

Removed completely the build requirements (wheel and setuptools) from pyproject.toml as they get installed as a build package requirement and otherwise it complains with:

I'm not knowledgable about pyproject.toml, but I think it could make more sense to remove the dedicated installation step of setuptools from the pip install --upgrade setuptools pip statement in the CI system, and let the pyproject.toml help it get installed. But, I really know to little about this to feel confident, but there is a warning bell saying that we may have made something that made sense now only work in the CI system but not work as intended outside the CI system.

One proxy test (the first one that starts traefik) is flaky as it sometimes fails to wait for traefik to load it's config. I increased the start timeout for that proxy but still didn't manage to make it stable, so suggest will leave as it is (at least for this PR) and try different approaches later on to fix this.

Sounds good!

What do you think @consideRatio ? Is there anything else I could do to improve this PR?

No =) Give the pyproject.toml part a think and then merge when have considered it :D

[manics]

I spent a few hours fighting with pyproject.toml over the weekend when I was updating jupyter-offlinenotebook.

It's related to PEP 517 and/or PEP 518.

From my limited understanding whether or not you need the build dependencies in pyproject.toml depends on how you build and install this package, since the build may be done in an isolated environment. pip install may behave differently from python setup.py, so we need to decide how people are likely to use this repo, and/or what we want to test or support. I concluded in my case it was necessary to have the build-dependencies in pyproject.toml.

[GeorgianaElena]

Thanks a lot @manics for pointing me in the right direction ❤️

The duplicate error was from the build package, which was reinserting the setuptools and wheel to the list of the requirements when no build-backend was provided in pyproject.toml (even though requires was there). This is fixed now, but not yet released, so I decided to explicitly specify it in the toml file.

Thanks a lot for your help @consideRatio and @manics. I'm going to merge this now.