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

Add tests for poetry #48

Merged
merged 1 commit into from
Feb 1, 2024
Merged

Add tests for poetry #48

merged 1 commit into from
Feb 1, 2024

Conversation

gmargaritis
Copy link
Contributor

Resolves #8

@gmargaritis gmargaritis requested a review from a team January 30, 2024 10:07
Copy link
Contributor

@parisk parisk left a comment

Choose a reason for hiding this comment

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

I would suggest taking a simpler approach on the CI level here, although the tests are 💯. A few suggestions:

  • Run tests on the whole matrix on every push, so that we know our commits work as expected.
  • If tests succeed, push image to GHCR tagged with ${github.sha}-{EXPECTED_RELEASE_TAG} (e.g. 95db419-1.7.1-python-3.12-bookworm)
  • On release, tag the existing Git SHA tagged image on GHCR with the release version (e.g. 1.7.1-python-3.12-bookworm)

.github/workflows/release.yml Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
@gmargaritis gmargaritis force-pushed the issue-008-add-tests branch 3 times, most recently from 0612abe to b11ea2d Compare January 31, 2024 15:31
@gmargaritis
Copy link
Contributor Author

@parisk Let me know how to proceed from here. What changes do we want to make in the release workflow?

Copy link
Contributor

@parisk parisk left a comment

Choose a reason for hiding this comment

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

I have added a few comments to tidy this up. We should also update the release workflow to just tag the images that already exist in the registry.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@gmargaritis
Copy link
Contributor Author

gmargaritis commented Feb 1, 2024

@parisk How does pulling of existing images based on the git sha work? Wouldn't the git sha be different for the commit that triggered the ci workflow, from the one that triggered the release workflow?

@parisk
Copy link
Contributor

parisk commented Feb 1, 2024

Nope, it's pretty much the same commit. All releases should happen on the main branch, for which the CI pipeline should always run: https://github.com/withlogicco/docker-poetry/pull/48/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR4-R6.

@gmargaritis gmargaritis force-pushed the issue-008-add-tests branch 2 times, most recently from 262ad78 to 0202d33 Compare February 1, 2024 11:02
Copy link
Contributor

@parisk parisk left a comment

Choose a reason for hiding this comment

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

Great work. A few more tweaks and we should be good to go!

.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@parisk parisk left a comment

Choose a reason for hiding this comment

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

LGTM 🦌

@gmargaritis gmargaritis merged commit ec2b3f2 into main Feb 1, 2024
10 checks passed
@gmargaritis gmargaritis deleted the issue-008-add-tests branch February 1, 2024 15:34
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.

Introduce tests
2 participants