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 check for dev containers #116

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

koppor
Copy link
Contributor

@koppor koppor commented Dec 4, 2023

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.

Copy link
Contributor

@krucod3 krucod3 left a 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.

.github/workflows/check-dev-containers.yml Outdated Show resolved Hide resolved
.github/workflows/check-dev-containers.yml Outdated Show resolved Hide resolved
@inf17101
Copy link
Contributor

inf17101 commented Dec 6, 2023

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 documentation_changes job inside the documentation workflow and copy and adapt to something similar like the following:

...
    - uses: dorny/paths-filter@v2.11.1
      id: filter
      with:
        filters: |
          devconainer:
              - '.devcontainer/**'
...

However, after the changes through this PR we need to adapt the self-service of Eclipse to make the status check required.

@koppor
Copy link
Contributor Author

koppor commented Dec 8, 2023

@koppor: You could take a look at the documentation_changes job inside the documentation workflow and copy and adapt to something similar like the following:

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).

@inf17101
Copy link
Contributor

@koppor: You could take a look at the documentation_changes job inside the documentation workflow and copy and adapt to something similar like the following:

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.")

@koppor
Copy link
Contributor Author

koppor commented Dec 14, 2023

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 Dockerfile automatically).

cancel-in-progress: true

jobs:
changes:
Copy link
Contributor

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?

Copy link
Contributor Author

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:

  1. avoidance of one checkout call
  2. avoidance of starting another job by GitHub workflows
  3. better maintenance, because cross-job dependencies are harder to maintain then inside-job dependencies (not scientifically proofed statement ^^)

Copy link
Contributor

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.

.github/workflows/check-dev-containers.yml Outdated Show resolved Hide resolved
Co-authored-by: Kaloyan <36224699+krucod3@users.noreply.github.com>
@koppor
Copy link
Contributor Author

koppor commented Jan 5, 2024

I currently don't have the time to continue here. Sorry for the efforts caused!

@koppor koppor closed this Jan 5, 2024
@krucod3 krucod3 mentioned this pull request Jan 8, 2024
3 tasks
@krucod3
Copy link
Contributor

krucod3 commented Jan 8, 2024

Sorry to hear that 🙁
Still, thanks a lot for the idea! We will take it over in #89

@koppor koppor reopened this Jan 12, 2024
@koppor
Copy link
Contributor Author

koppor commented Jan 12, 2024

I found some time to continue.

I made following changes:

  • Check for changes included in the jobs itself
  • Added caching for docker layers (improves build speed by approx one minute)
  • Added another job for a chained build: In case the base image changes, the final image is build on the current version of the base image in the repository.

@windsource
Copy link
Contributor

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.

@krucod3 krucod3 added the waiting-for-response Waiting for a response from contributor label Feb 8, 2024
@koppor
Copy link
Contributor Author

koppor commented Mar 6, 2024

@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 development or dev as "moving" tag? If you intend to release manually, for sure, fixed tags are OK. We could even include GitVersion to automatically set a meaningful SemVer of the Docker containers.

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.

@koppor
Copy link
Contributor Author

koppor commented Mar 6, 2024

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

@koppor
Copy link
Contributor Author

koppor commented Mar 6, 2024

@windsource

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

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.

@windsource
Copy link
Contributor

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.
So although this process is manually we expect to find build problems with the container very fast, as every committer usually uses new versions from main quite often and the CI build with every run. Having that said I am thinking of the benefit of automatic check for building the base container. You did quite some work for the new workflow but that needs to be maintained as well.
As mentioned before the manual build and push step for the base container is not optimal. But on the other hand we also do not change the base container that often. So currently we can live we that. What's you opinion?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-response Waiting for a response from contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants