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

workflows/build-ci-container: Make sure to only test local containers #120827

Merged
merged 1 commit into from
Jan 3, 2025

Conversation

tstellar
Copy link
Collaborator

The container test is run before we create the :latest tag, so we should not try testing this, otherwise it will pull the :latest tag from the github registry, and won't test the container we just built.

The container test is run before we create the :latest tag, so we should
not try testing this.
@llvmbot
Copy link
Member

llvmbot commented Dec 21, 2024

@llvm/pr-subscribers-github-workflow

Author: Tom Stellard (tstellar)

Changes

The container test is run before we create the :latest tag, so we should not try testing this, otherwise it will pull the :latest tag from the github registry, and won't test the container we just built.


Full diff: https://github.com/llvm/llvm-project/pull/120827.diff

1 Files Affected:

  • (modified) .github/workflows/build-ci-container.yml (+3-2)
diff --git a/.github/workflows/build-ci-container.yml b/.github/workflows/build-ci-container.yml
index 50729e0173506e..4fa0713b381ceb 100644
--- a/.github/workflows/build-ci-container.yml
+++ b/.github/workflows/build-ci-container.yml
@@ -59,8 +59,9 @@ jobs:
 
       - name: Test Container
         run: |
-          for image in ${{ steps.vars.outputs.container-name-tag }} ${{  steps.vars.outputs.container-name }}; do
-            podman run --rm -it $image /usr/bin/bash -x -c 'cd $HOME && printf '\''#include <iostream>\nint main(int argc, char **argv) { std::cout << "Hello\\n"; }'\'' | clang++ -x c++ - && ./a.out | grep Hello'
+          for image in ${{ steps.vars.outputs.container-name-tag }}; do
+            # Use --pull=never to ensure we are testing the just built image.
+            podman run --pull=never --rm -it $image /usr/bin/bash -x -c 'cd $HOME && printf '\''#include <iostream>\nint main(int argc, char **argv) { std::cout << "Hello\\n"; }'\'' | clang++ -x c++ - && ./a.out | grep Hello'
           done
 
   push-ci-container:

Copy link
Contributor

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

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

Why would it be trying to pull if the container exists locally?

The container shouldn't exist in the remote with the new tag.

Either way, seems like this fixes the issue, so approval for that.

@tstellar
Copy link
Collaborator Author

Why would it be trying to pull if the container exists locally?

The container shouldn't exist in the remote with the new tag.

Either way, seems like this fixes the issue, so approval for that.

You are correct that the --pull=never option is not strictly required, I just added it to be extra safe. Removing the testing for the :latest tag which was only available remotely is the part of the change the fixed my problem.

@boomanaiden154
Copy link
Contributor

Ah. Keeping it on there for the extra assurance definitely doesn't hurt.

@tstellar tstellar merged commit 1b5deae into llvm:main Jan 3, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants