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

ci(github): Revert to running funTests in a GitHub action container #8532

Closed
wants to merge 2 commits into from

Conversation

sschuberth
Copy link
Member

Please ensure that your pull request adheres to our contribution guidelines. Thank you!

@sschuberth sschuberth force-pushed the gh-container-job branch 3 times, most recently from a3e89b0 to 1ab48c7 Compare April 17, 2024 11:21
runs-on: ubuntu-22.04
container:
image: ${{ needs.funTest-docker-build.outputs.test_image_tag }}
options: --user 1001
Copy link
Member

Choose a reason for hiding this comment

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

Different jobs run on different machines, so you would have to publish the image somewhere. I think publishing PR builds next to the official builds in the registry is problematic, so I don't have a good idea where else they could be published. I was hoping the local registry could be a soluation, but that's also bound to the lifetime of a job, not a workflow.
The official docs have this solution, but it also doesn't work with container jobs: https://docs.docker.com/build/ci/github-actions/share-image-jobs/

Copy link
Member Author

Choose a reason for hiding this comment

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

I think publishing PR builds next to the official builds in the registry is problematic

Why? Wouldn't it be ok if we tagged them uniquely and removed them after the PR build again?

Copy link
Member

Choose a reason for hiding this comment

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

I guess I could live with that, even though I don't like those builds to appear when someone is looking at the packages in the GitHub UI. However, making sure that the image gets deleted even if some other steps fail might require implementing a custom action, I think composite actions don't support cleanup steps.
Another question is that of performance, of course the container job syntax is nicer than what we currently have, but it would have to be tested how uploading the image to the registry and then downloading it again compares to the current implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think composite actions don't support cleanup steps.

Wouldn't something like if: ${{ always() }} work, like we have it in static-analysis.yml?

Another question is that of performance, of course the container job syntax is nicer than what we currently have

I agree that performance matters, but this is not only about syntactic sugar. It also implicitly fixes the issue of restoring the unified diff for test failures, as environment variables get properly forwarded. Solving that problem led me to first look into why it worked before (with using container).

Copy link
Member

Choose a reason for hiding this comment

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

I think composite actions don't support cleanup steps.

Wouldn't something like if: ${{ always() }} work, like we have it in static-analysis.yml?

If that guarantees that the step is always executed that should be fine.

Another question is that of performance, of course the container job syntax is nicer than what we currently have

I agree that performance matters, but this is not only about syntactic sugar. It also implicitly fixes the issue of restoring the unified diff for test failures, as environment variables get properly forwarded. Solving that problem led me to first look into why it worked before (with using container).

Forwarding the kotest variable to Docker would also be trivial, so I would give performance the priority here.

@sschuberth sschuberth force-pushed the gh-container-job branch 3 times, most recently from 47d11b8 to e07cd23 Compare April 17, 2024 11:58
load: true
tags: ${{ env.TEST_IMAGE_TAG }}
push: true
tags: ort:run-${{ env.GITHUB_RUN_ID }}
Copy link
Member

@mnonnenmacher mnonnenmacher Apr 17, 2024

Choose a reason for hiding this comment

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

I think you have to use ${{ github.run_id }} or maybe ${{ GITHUB_RUN_ID }} or $GITHUB_RUN_ID, this page says those variables are not available from the env context, but it does not give good examples: https://docs.github.com/de/actions/learn-github-actions/variables#default-environment-variables

@sschuberth sschuberth force-pushed the gh-container-job branch 2 times, most recently from a8a5245 to 3e5fe85 Compare April 17, 2024 13:22
@sschuberth sschuberth force-pushed the gh-container-job branch 2 times, most recently from 448c121 to 2dbe353 Compare April 17, 2024 20:03
@sschuberth
Copy link
Member Author

The approaches tried out here do not work because

  • only whole jobs, not steps of a job, can run in containers, which means the container needs to be created in another job, and the only way to pass images between jobs would be via a registry, but GitHub does not allow to publish ORT's registry from PRs / forks.
  • uses pulls images at the start of a workflow, not only when it's needed, so there is no way to create the actual image beforehand.

@sschuberth sschuberth closed this Apr 22, 2024
@sschuberth sschuberth deleted the gh-container-job branch April 22, 2024 18:23
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.

2 participants