[no-relnote] Configure all GitHub actions as reusable workflow #915

ArangoGutierrez · 2025-02-10T14:39:28Z

No description provided.

ArangoGutierrez · 2025-02-10T14:41:34Z

elezar · 2025-02-10T15:10:05Z

.github/workflows/code_scanning.yaml

-  pull_request:
-    types:
-      - opened
-      - synchronize
-    branches:
-      - main
-      - release-*


Is there any reason to not use the "standard" triggers for this?

I think is a good first step to run the code scan before performing any extra steps.

That wasn't my question. Does it make sense to also run this on PRs?

elezar · 2025-02-10T15:10:44Z

.github/workflows/e2e.yaml

@@ -67,8 +73,8 @@ jobs:

      - name: Run e2e tests
        env:
-          IMAGE_NAME: ghcr.io/${LOWERCASE_REPO_OWNER}/container-toolkit
-          VERSION: ${COMMIT_SHORT_SHA}
+          IMAGE_NAME: ghcr.io/${{ inputs.lowercase_repo_owner }}/container-toolkit


Question: Isn't this always nvidia?

.github/workflows/ci.yaml

.github/workflows/image.yaml

elezar · 2025-02-10T15:14:47Z

.github/workflows/image.yaml

@@ -84,18 +93,6 @@ jobs:
    steps:
      - uses: actions/checkout@v4
        name: Check out code


What version is checked out here? Should we align this with the short sha?

would make sense in a follow-up PR. The defaults are ok, but moving forward when writing more reusable workflows, it will make more sense

https://github.com/actions/checkout?tab=readme-ov-file#usage

Shure. Could you please clarify which verison is checked out by default.

# The branch, tag or SHA to checkout. When checking out the repository that # triggered a workflow, this defaults to the reference or SHA for that event. # Otherwise, uses the default branch.

What I mean is, if this is triggered by a workflow call, is the SHA that is checked out the current version in the PR, or the SHA of the default branch.

is the SHA of the commit that triggered the action, unregarding of the branch (that's why copy-pr-bot works)

.github/workflows/ci.yaml

elezar · 2025-02-11T09:41:50Z

.github/workflows/ci.yaml

+          if [[ "${{ github.ref_name != 'main' && !startsWith(github.ref_name, 'release-') }}" == "true" ]]; then
+            echo "build_multi_arch_images=false" >> $GITHUB_OUTPUT
+          else
+            echo "build_multi_arch_images=true" >> $GITHUB_OUTPUT
+          fi


It may be clearer to invert the conditional:

Suggested change

if [[ "${{ github.ref_name != 'main' && !startsWith(github.ref_name, 'release-') }}" == "true" ]]; then

echo "build_multi_arch_images=false" >> $GITHUB_OUTPUT

else

echo "build_multi_arch_images=true" >> $GITHUB_OUTPUT

fi

if [[ "${{ github.ref_name == 'main' || startsWith(github.ref_name, 'release-') }}" == "true" ]]; then

echo "build_multi_arch_images=true" >> $GITHUB_OUTPUT

else

echo "build_multi_arch_images=false" >> $GITHUB_OUTPUT

fi

Actually, could we simplify this further to:

Suggested change

if [[ "${{ github.ref_name != 'main' && !startsWith(github.ref_name, 'release-') }}" == "true" ]]; then

echo "build_multi_arch_images=false" >> $GITHUB_OUTPUT

else

echo "build_multi_arch_images=true" >> $GITHUB_OUTPUT

fi

multi_arch_required="${{ github.ref_name == 'main' || startsWith(github.ref_name, 'release-') }}"

echo "build_multi_arch_images=${{ multi_arch_required }}" >> $GITHUB_OUTPUT

elezar · 2025-02-11T09:44:41Z

.github/workflows/ci.yaml

+    runs-on: ubuntu-latest
+    outputs:
+      version: ${{ steps.version.outputs.version }}
+      build_multi_arch_images: ${{ steps.build_multi_arch_images.outputs.value }}


Could this just be:

Suggested change

build_multi_arch_images: ${{ steps.build_multi_arch_images.outputs.value }}

build_multi_arch_images: ${{ github.ref_name == 'main' || startsWith(github.ref_name, 'release-') }}

and then we don't need the scripts below?

elezar · 2025-02-11T09:46:12Z

.github/workflows/image.yaml

@@ -84,18 +93,6 @@ jobs:
    steps:
      - uses: actions/checkout@v4
        name: Check out code


Shure. Could you please clarify which verison is checked out by default.

elezar · 2025-02-11T09:46:40Z

.github/workflows/image.yaml

-          VERSION: ${COMMIT_SHORT_SHA}
+          IMAGE_NAME: ghcr.io/nvidia/container-toolkit
+          VERSION: ${{ inputs.version }}
+          PUSH_ON_BUILD: ${{ inputs.push_on_build }}


What about?

Suggested change

PUSH_ON_BUILD: ${{ inputs.push_on_build }}

PUSH_ON_BUILD: true

I was going for reusability of the workflow file, but default to true is ok

When we need to, we can add push_on_build to the inputs and set a default value.

.github/workflows/image.yaml

elezar · 2025-02-11T09:47:48Z

.github/workflows/ci.yaml

+          echo "build_multi_arch_images=$multi_arch_required" >> $GITHUB_OUTPUT  
+
+  golang:
+    needs: code-scanning


What is the risk in running the golang tests and the code-scanning in parallel?

none. let me re-arrange

now we run the basic checks in parallel

elezar · 2025-02-11T10:49:49Z

.github/workflows/ci.yaml

+  prepare-variables:
+    runs-on: ubuntu-latest
+    outputs:
+      version: ${{ steps.version.outputs.version }}


Does the following work?

Suggested change

version: ${{ steps.version.outputs.version }}

version: ${{ github.sha.substring(0,8) }}

no, it doesn't

As a matter of interest, what error does it give? Do we have a reference of which functions are available to us? We do use startsWith, for example.

https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/evaluate-expressions-in-workflows-and-actions#functions

elezar · 2025-02-11T10:50:21Z

.github/workflows/ci.yaml

+  code-scanning:
+    uses: ./.github/workflows/code_scanning.yaml
+
+  prepare-variables:


Does it make sense to just call this variables?

elezar · 2025-02-11T11:04:02Z

.github/workflows/e2e.yaml

-          IMAGE_NAME: ghcr.io/${LOWERCASE_REPO_OWNER}/container-toolkit
-          VERSION: ${COMMIT_SHORT_SHA}
+          IMAGE_NAME: ghcr.io/nvidia/container-toolkit
+          VERSION: ${{ steps.vars.outputs.version }}


Suggested change

VERSION: ${{ steps.vars.outputs.version }}

VERSION: ${{ inputs.version }}

elezar · 2025-02-11T11:04:56Z

.github/workflows/e2e.yaml

@@ -67,8 +70,8 @@ jobs:

      - name: Run e2e tests
        env:
-          IMAGE_NAME: ghcr.io/${LOWERCASE_REPO_OWNER}/container-toolkit
-          VERSION: ${COMMIT_SHORT_SHA}
+          IMAGE_NAME: ghcr.io/nvidia/container-toolkit


If we do move e2e.yaml to a central repo, we would have to have this as an input.

(Out of scope for this PR).

elezar · 2025-02-12T09:16:24Z

If I recall correctly, the action results are tracked by SHA and I was wondering whether it makes sense to trigger the CodeQL and Golang checks on PRs as well? The motivation being that these would provide early feedback to contributors without requireing an /ok-to-test to trigger the PR copy.

Update: One place where this is required is for dependabot PRs. These do not "automatically" create a PR branch.

elezar · 2025-02-12T09:59:57Z

.github/workflows/code_scanning.yaml

-    branches:
-      - main
-      - release-*
+  workflow_call:


Given that we want these to run on PRs so that they trigger for dependabot we may want to update this to:

Suggested change

workflow_call:

workflow_call: {}

pull_request:

types:

- opened

- synchronize

branches:

- main

- release-*

elezar · 2025-02-12T10:00:15Z

.github/workflows/golang.yaml

-    branches:
-      - main
-      - release-*
+  workflow_call:


Given that we want these to run on PRs so that they trigger for dependabot we may want to update this to:

Suggested change

workflow_call:

workflow_call: {}

pull_request:

types:

- opened

- synchronize

branches:

- main

- release-*

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>

elezar

Thanks @ArangoGutierrez

Looks good.

ArangoGutierrez force-pushed the e2e_tests02 branch from 2399526 to 911ed71 Compare February 10, 2025 14:40

ArangoGutierrez requested a review from elezar February 10, 2025 14:41

ArangoGutierrez self-assigned this Feb 10, 2025

elezar reviewed Feb 10, 2025

View reviewed changes

.github/workflows/ci.yaml Outdated Show resolved Hide resolved

elezar reviewed Feb 10, 2025

View reviewed changes

.github/workflows/image.yaml Outdated Show resolved Hide resolved

elezar reviewed Feb 10, 2025

View reviewed changes

ArangoGutierrez force-pushed the e2e_tests02 branch from 911ed71 to 63ef912 Compare February 11, 2025 09:26

ArangoGutierrez requested a review from elezar February 11, 2025 09:26

elezar reviewed Feb 11, 2025

View reviewed changes

.github/workflows/ci.yaml Outdated Show resolved Hide resolved

elezar reviewed Feb 11, 2025

View reviewed changes

ArangoGutierrez force-pushed the e2e_tests02 branch 2 times, most recently from 87be0dd to d45f14d Compare February 11, 2025 09:45

elezar reviewed Feb 11, 2025

View reviewed changes

ArangoGutierrez force-pushed the e2e_tests02 branch from d45f14d to 1440638 Compare February 11, 2025 10:03

ArangoGutierrez requested a review from elezar February 11, 2025 10:05

ArangoGutierrez force-pushed the e2e_tests02 branch 2 times, most recently from 84b9711 to 992c72f Compare February 11, 2025 10:31

elezar reviewed Feb 11, 2025

View reviewed changes

ArangoGutierrez force-pushed the e2e_tests02 branch from 992c72f to 987f91e Compare February 11, 2025 19:11

ArangoGutierrez requested a review from elezar February 11, 2025 19:11

ArangoGutierrez force-pushed the e2e_tests02 branch from 987f91e to e280018 Compare February 11, 2025 19:21

elezar reviewed Feb 12, 2025

View reviewed changes

[no-relnote] COnfigure all GitHub actions as reusable workflow

27f1738

Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>

ArangoGutierrez force-pushed the e2e_tests02 branch from e280018 to 27f1738 Compare February 12, 2025 10:06

ArangoGutierrez requested a review from elezar February 12, 2025 10:06

elezar approved these changes Feb 12, 2025

View reviewed changes

ArangoGutierrez merged commit 5db5ddb into NVIDIA:main Feb 12, 2025
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[no-relnote] Configure all GitHub actions as reusable workflow #915

[no-relnote] Configure all GitHub actions as reusable workflow #915

ArangoGutierrez commented Feb 10, 2025

ArangoGutierrez commented Feb 10, 2025

elezar Feb 10, 2025

ArangoGutierrez Feb 11, 2025

elezar Feb 11, 2025

elezar Feb 10, 2025

elezar Feb 10, 2025

ArangoGutierrez Feb 11, 2025

elezar Feb 11, 2025

ArangoGutierrez Feb 11, 2025

elezar Feb 11, 2025

ArangoGutierrez Feb 12, 2025

elezar Feb 11, 2025

elezar Feb 11, 2025

elezar Feb 11, 2025

elezar Feb 11, 2025

ArangoGutierrez Feb 11, 2025

elezar Feb 11, 2025 •

edited

Loading

elezar Feb 11, 2025

ArangoGutierrez Feb 11, 2025

ArangoGutierrez Feb 11, 2025

ArangoGutierrez Feb 11, 2025

elezar Feb 11, 2025

ArangoGutierrez Feb 11, 2025

elezar Feb 12, 2025

ArangoGutierrez Feb 12, 2025

elezar Feb 11, 2025

elezar Feb 11, 2025

elezar Feb 11, 2025

ArangoGutierrez Feb 11, 2025

elezar commented Feb 12, 2025 •

edited

Loading

elezar Feb 12, 2025

elezar Feb 12, 2025

elezar left a comment

	build_multi_arch_images: ${{ steps.build_multi_arch_images.outputs.value }}
	build_multi_arch_images: ${{ github.ref_name == 'main' \|\| startsWith(github.ref_name, 'release-') }}

	PUSH_ON_BUILD: ${{ inputs.push_on_build }}
	PUSH_ON_BUILD: true

	version: ${{ steps.version.outputs.version }}
	version: ${{ github.sha.substring(0,8) }}

	VERSION: ${{ steps.vars.outputs.version }}
	VERSION: ${{ inputs.version }}

[no-relnote] Configure all GitHub actions as reusable workflow #915

[no-relnote] Configure all GitHub actions as reusable workflow #915

Conversation

ArangoGutierrez commented Feb 10, 2025

ArangoGutierrez commented Feb 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elezar Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elezar commented Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elezar left a comment

Choose a reason for hiding this comment

elezar Feb 11, 2025 •

edited

Loading

elezar commented Feb 12, 2025 •

edited

Loading