-
Notifications
You must be signed in to change notification settings - Fork 166
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 and CI #34
Add tests and CI #34
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you.
Lots of suggestions:
eac6dca
to
92c8a8a
Compare
Changes made @yajo, apart from the comment above. As a reference, see https://docs.github.com/en/free-pro-team@latest/packages/getting-started-with-github-container-registry/migrating-to-github-container-registry-for-docker-images#updating-your-github-actions-workflow ( |
b49f3ef
to
4779acb
Compare
4779acb
to
d2dad86
Compare
I've made come changes @yajo. Still WIP so pls tell me if this goes in the direction you meant. |
Not exactly. Let me write that code for you. |
I pushed the initial structure. It is probably failing, but I hope you can continue your WIP on top of it. I also added a very important point to your checklist in first comment: Make sure the same image that is tested is pushed. |
9109d34
to
3cf4ee6
Compare
Thank you for the suggestions @yajo |
Is this ready to review? |
I am still working on adding the auto config generator script as an entrypoint and making sure the same image that is tested is pushed. I was thinking on marking it as ready when that is done 😃 Tests are ready for review, however. |
Ended up removing:
|
898582c
to
f66b9c5
Compare
We seem to be hitting docker/buildx#59. I applied docker/buildx#59 (comment) to build the image only in the runner's platform for testing purposes and then build the rest when it is time to push. This is a limitation of docker buildx when exporting to local docker, which we need to test the image. It is not the best but we would only test one version of the image anyway (the one corresponding to the runner's platform), right? 🤷♂️ |
f66b9c5
to
d2416e8
Compare
on: | ||
pull_request: | ||
push: | ||
branches: | ||
- master | ||
workflow_dispatch: | ||
inputs: | ||
pytest_addopts: | ||
description: | ||
Extra options for pytest; use -vv for full details; see | ||
https://docs.pytest.org/en/latest/example/simple.html#how-to-change-command-line-options-defaults | ||
required: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think this is the time to implement #33 (comment)?
I'm developing for TT25794 a new python lib that will help know that you're currently in the latest tag. I'll notify you when it's done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good suggestion. I agree that it makes sense now. I'll wait for your call then.
Builds the image (in single arch) before testing Loads the image into local docker (See https://github.com/docker/build-push-action#export-image-to-docker) Rebuilds and pushes the final image in multi-arch at the end.
08c7cff
to
dc0b60e
Compare
This reverts commit dc0b60e and allows using `--prebuild` CLI flag for pytest when doing local tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very good.
I think it's better to not block it due to #34 (comment) because that'll take a bit of time still.
Takes from #14 and start apllying suggestions and improvements.
So far:
POST
ruleTT26468