Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

Open
f0rki wants to merge 1 commit intopapis:mainfrom
f0rki:ci-container-builds
Open

Enabled two new CI workflows to build, test, (and release) containers.#612
f0rki wants to merge 1 commit intopapis:mainfrom
f0rki:ci-container-builds

Conversation

@f0rki
Copy link
Contributor

Test the nix and docker-based install methods.

  • container-build.yml - does the docker magic
    • build a docker container with the defaultDockerfile and check whethermake ci-test works.
    • build a "release" container that is pushed toghcr.io/papis/papis, i.e., papis packaged as a container.
      • this container is tested by running a couple of papis commandstools/run-integration-test.sh
  • nix-flake.yml - does the nix magic
    • runsnix build - checks the flake build
    • runsnix 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 runpapis serve on your home server this might be a convenient thing to run.

Copy link
Collaborator

@alexfiklalexfikl 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?

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
ContributorAuthor

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 runningpytest if those succeed.

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

Copy link
ContributorAuthor

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 onubuntu-latest with latest python in a separate step before running all tests/lints with the matrix job.

@f0rki
Copy link
ContributorAuthor

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

@f0rkif0rkiforce-pushed theci-container-builds branch 2 times, most recently from0134f83 toee9d4f4CompareJuly 24, 2023 11:14
@alexfikl
Copy link
Collaborator

alexfikl commentedAug 28, 2023•
edited
Loading

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 notubuntu-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
ContributorAuthor

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.

alexfikl reacted with thumbs up emoji

@f0rki
Copy link
ContributorAuthor

f0rki commentedAug 29, 2023•
edited
Loading

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

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

alexfikl reacted with thumbs up emoji

This was referencedAug 29, 2023
@alexfikl
Copy link
Collaborator

alexfikl commentedAug 31, 2023•
edited
Loading

@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 toghcr.io. I don't have access to do that, so whenever you get a chance it would be cool to get it in 😁

f0rki reacted with thumbs up emoji

@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
ContributorAuthor

rebased and pushed fixes. should work now again.

alexfikl reacted with heart emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@alexfiklalexfiklalexfikl left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@f0rki@alexfikl

Comments

[8]ページ先頭
©2009-2026 Movatter.jp