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

feat: Run tests in Docker #63

Closed
wants to merge 1 commit into from

Conversation

sinopeus
Copy link
Collaborator

  • Build Docker CI image
  • Run tests in built Docker CI image
  • Use GitHub Actions Docker cache to speed up future Docker builds
  • Drive-by action upgrades :-)

- Build Docker CI image
- Run tests in built Docker CI image
- Use GitHub Actions Docker cache to speed up future Docker builds
- Drive-by action upgrades :-)
@sinopeus sinopeus marked this pull request as draft June 18, 2022 22:04
@sinopeus
Copy link
Collaborator Author

Still need to fix private package repo support + other things that are a bit broken. But that will be for another time, just wanted to put this out there 🙂

@sinopeus sinopeus requested a review from lsorber June 18, 2022 22:06
@lsorber
Copy link
Member

lsorber commented Jun 20, 2022

Still need to fix private package repo support + other things that are a bit broken. But that will be for another time, just wanted to put this out there 🙂

OK, let me know when it's ready for review!

Copy link
Member

@lsorber lsorber left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I know it's not ready yet, but I already did a first review pass.

- name: Set up Python {% raw %}${{ matrix.python-version }}{% endraw %}
uses: actions/setup-python@v3
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v2
Copy link
Member

Choose a reason for hiding this comment

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

For my information: why is this action needed? I believe docker/build-push-action already uses BuildKit, if that's the reason.

path: .venv/
key: {% raw %}venv-${{ runner.os }}-py${{ matrix.python-version }}-${{ hashFiles('poetry.lock') }}{% endraw %}
context: .
target: dev
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we be building ci here? The dev image is much larger because it contains developer tools that in principle aren't necessary in CI.

strategy:

matrix:
python-version: ["{{ cookiecutter.python_version }}"]
Copy link
Member

Choose a reason for hiding this comment

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

Although I'm all for using Docker, we do have to be aware that this PR removes the ability to test in a matrix, which is a bit of a downgrade for Python packages (for Python applications that shouldn't matter too much).

cache-from: type=gha
cache-to: type=gha,mode=max
tags: |
ghcr.io/${{ github.repository }}/ci:${{ github.sha }}
Copy link
Member

Choose a reason for hiding this comment

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

What is this SHA based on? Shouldn't we base it off of Dockerfile, poetry.lock, and pyproject.toml as we do in GitLab CI/CD?

@@ -38,29 +38,27 @@ jobs:
- name: Log in to the Docker registry
uses: docker/login-action@v1
with:
registry: {% raw %}${{ env.REGISTRY }}{% endraw %}
username: {% raw %}${{ github.actor }}{% endraw %}
password: {% raw %}${{ secrets.GITHUB_TOKEN }}{% endraw %}
Copy link
Member

Choose a reason for hiding this comment

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

We need the {% raw %} bits here because otherwise Cookiecutter will treat these as values that should be interpolated.

@sinopeus
Copy link
Collaborator Author

This is actually similar to Tanguy's PR#8, in a simplified form. Let's see what we keep from both (once comments are handled). Part of the intention was to give Renaud a starting example for converting a GitLab CI/CD pipeline to GitHub Actions as well, which is why this isn't really complete 🙂

@lsorber
Copy link
Member

lsorber commented Jul 18, 2022

I suppose this PR is obsolete now that we've merged #65, or is there anything in this one we still want to retain? Apologies for the double effort!

@sinopeus sinopeus closed this Aug 8, 2022
@sinopeus sinopeus deleted the xga-run-tests-in-docker branch August 8, 2022 17:28
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

3 participants