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

Enabled two new CI workflows to build, test, (and release) containers. #612

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

f0rki
Copy link
Contributor

@f0rki f0rki commented Jul 23, 2023

Test the nix and docker-based install methods.

  • container-build.yml - does the docker magic
    • build a docker container with the default Dockerfile and check whether make ci-test works.
    • build a "release" container that is pushed to ghcr.io/papis/papis, i.e., papis packaged as a container.
      • this container is tested by running a couple of papis commands tools/run-integration-test.sh
  • nix-flake.yml - does the nix magic
    • runs nix build - checks the flake build
    • runs nix develop - checks whether pytest passes in the nix development environment.

Packaging papis in docker is a nice thing to have and quite easy with github's container registry (ghcr.io). For example, if you like to run papis serve on your home server this might be a convenient thing to run.

Copy link
Collaborator

@alexfikl alexfikl left a comment

Choose a reason for hiding this comment

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

Left a few questions, but overall this looks good to me!

Does this require some special access to publish the Docker images?

.github/workflows/container-build.yml Show resolved Hide resolved
.github/workflows/container-build.yml Show resolved Hide resolved
Comment on lines 19 to 23
fail-fast: False
# fail fast only on pull-requests!
fail-fast: ${{ github.event_name == 'pull_request' }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change? The CI often fails on Windows but not Linux or on some python versions and not others, so it's very useful to let it all run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I wanted to reduce redundant failures, e.g. because PRs forgot to format or because of some trailing whitespace. But I can remove that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's a fair point, but probably better handled by just separating the static checks (flake8, mypy) and only running pytest if those succeed.

papis tests are pretty fast, so we never bothered too much :\

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok I separated them. Now the lints are run first on ubuntu-latest with latest python in a separate step before running all tests/lints with the matrix job.

.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/nix-flake.yml Outdated Show resolved Hide resolved
.github/workflows/nix-flake.yml Outdated Show resolved Hide resolved
tests/test_config.py Outdated Show resolved Hide resolved
@f0rki
Copy link
Contributor Author

f0rki commented Jul 24, 2023

Yeah I think you have to enable that workflows can write to the repo/registry in the Settings.

@f0rki f0rki force-pushed the ci-container-builds branch 2 times, most recently from 0134f83 to ee9d4f4 Compare July 24, 2023 11:14
@alexfikl
Copy link
Collaborator

alexfikl commented Aug 28, 2023

I'm sorry for letting this fall off the radar for quite a while now. Maybe we can get some of it in!
From a quick skim:

  1. I don't have access to add the required Docker permissions to make that workflow work, so that will have to wait.
  2. Splitting the linting in the main CI doesn't sound like a good idea. Time-wise, the CI is pretty fast and the OS/version matrix takes the most time. Otherwise, it's better to pass linting on all versions we support so contributors can have a clean lint even if their system is not ubuntu-latest. WONTFIX
  3. There are many unrelated fixes for various things in here (from ruff?). Linting fixes #649
  4. The nix stuff is done. ci: add workflow for testing nix flake #647

3 and especially 4 can go in right away, but they will have to be in separate PRs, since I can't merge this wholesale. Do you have any time to work on these? If not, I can slowly cherry pick stuff and merge it.

@f0rki
Copy link
Contributor Author

f0rki commented Aug 29, 2023

If not, I can slowly cherry pick stuff and merge it.

Yes feel free to cherry pick the commits you think are worth merging now.

@f0rki
Copy link
Contributor Author

f0rki commented Aug 29, 2023

There are many unrelated fixes for various things in here (from ruff?).

Yes there is a commit (e9cb62b) that adds things from ruff --fix

@alexfikl
Copy link
Collaborator

alexfikl commented Aug 31, 2023

@f0rki Squashed and rebased this to only contain the docker stuff, since everything else should be in main already.

@alejandrogallo According to @f0rki, this requires adding some permissions to the settings and maybe some other things before it can actually get automagically published to ghcr.io. I don't have access to do that, so whenever you get a chance it would be cool to get it in 😁

@alexfikl
Copy link
Collaborator

This seems to be currently failing: https://github.com/f0rki/papis/actions/runs/6040713352/job/16392121597 (possibly my bad :(

@f0rki
Copy link
Contributor Author

f0rki commented Sep 11, 2023

rebased and pushed fixes. should work now again.

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

2 participants