-
Notifications
You must be signed in to change notification settings - Fork 21
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 check for dev containers #116
base: main
Are you sure you want to change the base?
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.
Reviewed and added some comments.
When we are talking about fast builds, would it be possible to run the workflow only if one of the files changed? Maybe we can take over the checks in the documentation workflow.
@koppor: You could take a look at the
However, after the changes through this PR we need to adapt the self-service of Eclipse to make the status check required. |
I used the path filtering directly (as outlined at https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#onpushpull_requestpull_request_targetpathspaths-ignore). Then, this action does not run at all if no changes were made. I think, this is even more efficient. However, I am not sure how this relates to "required" actions to pass. Needs to be tried out... - In case it works, this would also be helpful for automatic dependency updates including an auto merge (future work). |
We cannot use the path filtering directly if we mark this job as required status check later. Then the user is accidentally blocked from merging, because a workflow trigger condition that is skipped (due to no changes were made to the specified files) is marked as a pending job and would prevent the user from merging the PR. This is why you must transfer a workflow condition to a job condition. A skipped job condition would not mark a status check as pending even if the workflow itself is marked as required status check. The suggested Github action (in the documentation workflow) executes the path filter as a job condition. Then merges are not blocked. This is also mentioned in your posted link (please have a look into the "note" box, where Github doc says explicitly "Note: You should not use path filtering to skip workflow runs if the workflow has been configured to be required.") |
I parallelized the check for base and the "real" image, because the "real" image needs an image from the registry (and does not depend on the build base image). Refs #126 (because an enhancement of this workflow will publish a release of the DevContainer upon successful build and probably also update the |
cancel-in-progress: true | ||
|
||
jobs: | ||
changes: |
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.
If we are splitting the build steps for performance, why not splitting the checks too?
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.
I unzip that comment for me:
The time for checking is building the action and the checkout. The check itself is fast.
Thus, when moving the changes-check to the respective job, we gain performance in three ways:
- avoidance of one checkout call
- avoidance of starting another job by GitHub workflows
- better maintenance, because cross-job dependencies are harder to maintain then inside-job dependencies (not scientifically proofed statement ^^)
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.
The drawback is, that every steps then needs an if:
statement which make the code a little bit harder to read.
Co-authored-by: Kaloyan <36224699+krucod3@users.noreply.github.com>
I currently don't have the time to continue here. Sorry for the efforts caused! |
Sorry to hear that 🙁 |
I found some time to continue. I made following changes:
|
I just wonder whether we can also use this action to build the base image for the dev container and how long this would take for building an amd64 and arm64 image using public Github Action runners? Probably the build would have to be started manually and then a parameter for the tag needs to be passed. First the check should run and finally the base image needs to be pushed. Then the tag name probably needs to be entered manually in the derived Dockerfile using the base image. |
@windsource Sure. We can pair up, since I do not have your docker registry credentials. Someone with Ankaios registry push permissions should join the team. Versioning is hard. We all know. Maybe using See line 114 (https://github.com/eclipse-ankaios/ankaios/pull/116/files#diff-d81ffa0ff90993ddb0df7ff9678e69ffa70438cace8f1a34804f3d03de55586eR114) that I even changed the reference of the next Dockerfile to use the previously build one. Since this is a hobby contribution of me, I would really appreciate it if I could discuss with someone than guess what the architects of Ankaios thought with their containers and especially the container versioning, interpret that, build a separate CI/CD pipeline, push to my own Docker profile, propose something, ask to put credentials somewhere and then incorporate feedback etc. -- It's OK for me if this is the only way, then I'll do as soon I have time. |
@windsource arm64 is IMHO only possible with BuildJet easily: https://buildjet.com/for-github-actions/docs/about/pricing#arm. I successfully run that in another project. (More background at actions/runner-images#5631 (comment)) |
Currently, the build is done on each change of the image. And @inf17101 even proposed to make the check mandatory: #116 (comment). One can also trigger builds if a tag is set. I don't know about your versioning policy in detail. I would version the dev containers along with the releases of Ankaios. Thus, at each "tag" of Ankaois, a bulid and push is triggered. I am happy to contribute, but I need more guidance about your wishes and also credentials for the Docker registry. |
Hi @koppor, sure, let's discuss the usage of containers in development and CI for Ankaios. So current we have the base container which serves as base of the dev container during development and also for CI builds. So the base container needs to contain all the tools that are also used for CI builds. Currently we build the base container manually. So before a PR with a change in the base container can be merged, the base container needs to be build manually by a committer and pushed to ghcr. In this step we also increase the version number of the container. Reproducible builds are super important for me. For that reason we do not use latest or dev as label. In order to also support Mac with Apple Silicon we create multi-arch container for amd64 and arm64. As those builds take a long time (one arch always needs to be emulated) we usually use Mac with Apple Silicon for the build as Rosetta does the emulation very efficient. |
This PR adds a CI heck for DevContainer builds as a double check if a DevContainer builds.
Maybe, it can even be used as basis to do more build checks - based on the build dev container.
Definition of Done
The PR shall be merged only if all items mentioned in CONTRIBUTING.md have been followed. In case an item is not applicable as described, please provide a short explanation in the description.