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

Extract shared test workflows and action #1037

Closed
wants to merge 1 commit into from

Conversation

danielrbradley
Copy link
Member

@danielrbradley danielrbradley commented Jul 12, 2024

Fixes #1034

Related to #936

Stacked on top of:

The "test" and "test examples" share most of their setup except for the actual test command to run so we can extract this into a "test-setup" composite action.

Only the "run-acceptance-tests" workflow runs the example tests so we only call it from there. The main, prerelease and release workflows only call the main "test" workflow.

Reorder the top-level workflow to be closer to the order of execution to make it easier to find the code when looking at an execution run.

@danielrbradley danielrbradley self-assigned this Jul 12, 2024
@danielrbradley
Copy link
Member Author

There was no significant diff between the test job in the main, prerelease and release workflows.

This is the diff between the test job in the run-acceptance-tests and main workflows:

@@ -5,10 +5,8 @@
 
 jobs:
   test:
-    if: github.event_name == 'repository_dispatch' ||
-      github.event.pull_request.head.repo.full_name == github.repository
     name: test
-    needs:
+    needs: 
       - prerequisites
       - build_sdk
     permissions:
@@ -28,17 +26,10 @@
 #{{- end }}#
     - name: Checkout Repo
       uses: #{{ .Config.actionVersions.checkout }}#
+#{{- if .Config.checkoutSubmodules }}#
       with:
-        ref: ${{ env.PR_COMMIT_SHA }}
-        #{{- if .Config.checkoutSubmodules }}#
         submodules: #{{ .Config.checkoutSubmodules }}#
-        #{{- end }}#
-    - name: Checkout p/examples
-      if: matrix.testTarget == 'pulumiExamples'
-      uses: #{{ .Config.actionVersions.checkout }}#
-      with:
-        repository: pulumi/examples
-        path: p-examples
+#{{- end }}#
     - name: Setup tools
       uses: ./.github/actions/setup-tools
       with:
@@ -104,13 +95,8 @@
 #{{ .Config.actions.preTest | toYaml | indent 4 }}#
 #{{- end }}#
     - name: Run tests
-      if: matrix.testTarget == 'local'
-      run: cd examples && go test -v -json -count=1 -cover -timeout 2h -tags=${{
-        matrix.language }} -skip TestPulumiExamples -parallel 4 . 2>&1 | tee /tmp/gotest.log | gotestfmt
-    - name: Run pulumi/examples tests
-      if: matrix.testTarget == 'pulumiExamples'
       run: cd examples && go test -v -json -count=1 -cover -timeout 2h -tags=${{
-        matrix.language }} -run TestPulumiExamples -parallel 4 . 2>&1 | tee /tmp/gotest.log | gotestfmt
+        matrix.language }} -parallel 4 . 2>&1 | tee /tmp/gotest.log | gotestfmt
     strategy:
       fail-fast: false
       matrix:
@@ -120,8 +106,6 @@
         - dotnet
         - go
         - java
-        #{{- if .Config.testPulumiExamples }}#
-        testTarget: [local, pulumiExamples]
-        #{{- else }}#
-        testTarget: [local]
-        #{{- end }}#
+#{{- if .Config.extraTests }}#
+#{{ .Config.extraTests | toYaml | indent 2 }}#
+#{{ end }}#

The significant changes here is the addition of testing examples from p/examples during PR workflows (see #823).

There's also a change in the checkout ref, but this should be safely removed and use the default checkout behaviour on PRs.

@danielrbradley danielrbradley marked this pull request as draft July 12, 2024 13:31
The "test" and "test examples" share most of their setup except for the actual test command to run so we can extract this into a "test-setup" composite action.

Only the "run-acceptance-tests" workflow runs the example tests so we only call it from there. The main, prerelease and release workflows only call the main "test" workflow.

Reorder the top-level workflow to be closer to the order of execution to make it easier to find the code when looking at an execution run.

Remove explicit ref checkout during tests

This ref isn't checked out for any other step so shouldn't be used here either.

Remove left-over test-example condition

We should be allowed to run this from other workflows. This condition is just needed within the run-acceptance-tests top-level workflow.

Checkout before calling composite action

The code has to be checked out to call a local action.

Fix missing shells

Add env for sub-workflows

Fix secrets access in composite action

Secrets are not available and must be passed as explicit inputs.

This must also be fixed in any providers specifying arbitrary code in their actions.preTest ci-mgmt config.

Always ensure upstream before tests

This is hard-coded into lots of provider's preTest config.

Fix missing token secret

Only test examples if enabled

Fix missing upstream shell

Apply to nightly-test workflow
@danielrbradley danielrbradley force-pushed the refactor-test-workflow branch from bdbb340 to ee91ccb Compare July 12, 2024 15:24
@danielrbradley danielrbradley changed the base branch from master to remove-actions-hooks July 12, 2024 15:26
@mjeffryes mjeffryes modified the milestone: 0.107 Jul 24, 2024
danielrbradley added a commit that referenced this pull request Jul 24, 2024
This will allow us to remove all of our `preTest` hooks in provider
ci-mgmt configs - which we'd like to get rid of to avoid arbitary code
injection into workflows which make them fragile and hard to refactor.
See also #1037

Related to #936

## Add standalone option for running provider integration tests

Set `integrationTestProvider: true` to run `go test [...] -tags=${{
matrix.language }}` in the `provider` directory.

Example of existing preTest usage:
https://github.com/pulumi/pulumi-gcp/blob/92b64b51bfb05fc0d2d7b9c1cbcafe7de8a6f7f3/.ci-mgmt.yaml#L43

### Always prepare upstream before testing

This step is safe to run, as we already do before the Go build steps
elsewhere. The upstream target is always there but might be a no-op.

## Add SSH setup option

Set `sshPrivateKey: ${{ secrets.PRIVATE_SSH_KEY_FOR_DIGITALOCEAN }}` to
set up SSH for testing.

Example of docker use of preTest:
https://github.com/pulumi/pulumi-docker/blob/fc1d68f823c34cef72e34e060b093230e21fc636/.ci-mgmt.yaml#L35

# Migration

1. Merge this PR with intent-focused config options
2. Open PRs to the ~28 providers to replace the `preTest` hook with
alternative settings
- Set `integrationTestProvider: true` on all providers which run
integration tests via the provider module
   - Use `setup-script` for arbitrary bash commands before tests 
- Set `sshPrivateKey: ${{ secrets.PRIVATE_SSH_KEY_FOR_DIGITALOCEAN }}`
for docker
4. Remove the `actions` config completely

Existing usage of `actions` hooks (there is no use of `preBuild` - only
`preTest`):
https://github.com/search?q=org%3Apulumi+path%3A.ci-mgmt.yaml+%22actions%3A%22&type=code
@blampe
Copy link
Contributor

blampe commented Dec 13, 2024

#1214

@blampe blampe closed this Dec 13, 2024
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.

Extract test shared workflow
3 participants