Integration tests in forks #5856

jsoref · 2022-09-08T04:36:18Z

Note: Fixes for the online documentation of the current Stack release (https://docs.haskellstack.org/en/stable/) should target the 'stable' branch, not the 'master' branch.

Please include the following checklist in your pull request:

Any changes that could be relevant to users have been recorded in ChangeLog.md.
The documentation has been updated, if necessary

Please also shortly describe how you tested your change. Bonus points for added tests!

Without these changes, the following things can go wrong in forks:

when one pulls in an update to master, the the workflow will run, and then get stuck waiting for a self-hosted-runner
if one had a self-hosted-runner, signing would fail unless one also had a signing secret
if one had both of the above and had an organization that restricts permissions, then the workflow could fail because it doesn't ask for write permissions

Beyond that, the GitHub's actions/create-release is unsupported, so this PR switches to a maintained action (one that actually documents the permission required required to create a release...).

Note: you should not merge this to this repository without creating a secret named SELF_HOSTED_RUNNERS that has a value like arm64 (well, you can, but it'd result in arm64 builds being skipped until the secret is added).

Here's a run with the secret set (that would have passed if I had such a runner):
https://github.com/check-spelling/stack/actions/runs/3012421271

Here's a run without the secret set that passes:
https://github.com/check-spelling/stack/actions/runs/3012476622

mpilgrem · 2022-09-08T11:32:27Z

@jsoref, many thanks. I am new to GitHub Actions, but it looks good to me. You will see on Slack that @snoyberg has offered to create the necessary SELF_HOSTED_RUNNERS secret with value arm64.

mpilgrem · 2022-09-08T12:33:24Z

@jsoref, we apply linting of YAML files and it is 'opinionated' when it comes to indentation. When the rest of the CI is complete, please can you fix the indentation that it complains about to be consistent with the approach in the rest of the YAML file.

mpilgrem · 2022-09-08T12:38:09Z

@jsoref, on your comments on Slack about testing, once this completes the CI. I will merge it and then test it here by creating a dummy 'version' tag (which I will later delete) and see if it creates a draft release as intended. Can I leave it to you to test that it works as intended in a fork of the repository?

jsoref · 2022-09-08T12:48:49Z

Sure.

jsoref · 2022-09-08T13:04:20Z

I should note that the integration tests running now aren't testing this code at all, the only thing that would are things like the linter (see below). GitHub (which is the only thing that directly uses these files) doesn't use the versions of the files as present in PRs, because that's a security risk.

As such, the PR is otherwise functionally equivalent to a test for flaky tests.

The reason that I didn't notice the linter failure in my testing is that I didn't make a PR for this (I did use the PR mechanism to validate the spelling code). I'd argue that the linter should probably lint all branches (but not tags). The way my workflow does this is:

  push:
    branches: ["**"]
    tags-ignore: ["**"]

mpilgrem · 2022-09-08T14:02:59Z

@jsoref, please can you explain further. The GitHub Actions are following the changed workflow files in the pull request, as I would expect, but it is not finding any of the secrets. If workflows in pull requests can't access secrets, then how does the proposed CI work?

jsoref · 2022-09-08T14:08:39Z

Once the PR is merged, the version here will be the one that would run the next time a commit is added to master, or similar it would run.

But while in the context of this PR, the workflow being changed here isn't run.

On secret access, you're right. Oops, this won't work "well enough". It'd work for anyone self-testing, but indeed, it won't help for PRs from forks, and while it's possible to make it work, the risk entailed in that change isn't worth it. Lemme rebase and include a version that special cases this specific repository commercialhaskell/stack -- it'll mean that you won't need the secret, but that anyone in a fork who chooses to set up a self-hosted runner will be able to set the secret themselves and use it.

mpilgrem · 2022-09-08T14:11:40Z

On the changed workflow, I don't think that is correct. You can see from this pull request's checks that it is the changed workflow that is being run. I think the only stopping block is the access to the secrets. This https://docs.github.com/en/actions/security-guides/encrypted-secrets#accessing-your-secrets suggests that only people who have permission to edit the workflow file can use secrets in that workflow file.

jsoref · 2022-09-08T14:31:26Z

Strange. I could have sworn that workflows from the PR branch weren't used. I wonder if that changed w/ the "require owner approval" bits...

Anyway, I think I'm done writing the logic change (got interrupted by a work question).

jsoref · 2022-09-08T14:43:46Z

.github/workflows/integration-tests.yml

+      id: runners
+      shell: bash
+      env:
+        SELF_HOSTED_RUNNERS: ${{ secrets.SELF_HOSTED_RUNNERS || (github.repository_owner == 'commercialhaskell' && 'arm64') }}


GitHub workflows/actions don't have a ?: operator, but they do have || and && operators that work more or less as you'd expect them to in JavaScript.

If a secret doesn't exist, or isn't accessible (because a PR is from a different repository in the fork network), then the secrets.WHATEVER will evaluate to '' which is treated as false for conditional logic.

Thus, if this is running due to a push in a repository (or a pull_request from the same repository) with the secret set, it'll use the secret.

Otherwise, when the left side of the || evaluates to empty it'll use the right side of the ||, which then compares the github.repository_owner which if it's running in this repository should be commercialhaskell, making the left side of the && true, which will result in it picking arm64. Technically if this fails, the resulting value will be "false" (the output of the logic was false, but environment variables are coerced to strings). Since the contains() function calls below compare against "arm64", that's harmless as it won't match.

jsoref · 2022-09-08T14:52:16Z

Fwiw, here's a run w/o the secret set: https://github.com/check-spelling/stack/actions/runs/3015938080

And here's a run w/ the secret set: https://github.com/check-spelling/stack/runs/8252116879?check_suite_focus=true -- if you expand Check for self-hosted runners, you'll see:

Run echo "::set-output name=runners::$SELF_HOSTED_RUNNERS"
##[debug]/usr/bin/bash --noprofile --norc -e -o pipefail /home/runner/work/_temp/fa19460b-d927-4ef4-bb3f-539cb4cad371.sh
::set-output name=runners::***
##[debug]steps.runners.outputs.runners='***'
::set-output name=***::"[\"self-hosted\", \"linux\", \"ARM64\"]"
##[debug]steps.runners.outputs.***='"[\"self-hosted\", \"linux\", \"ARM64\"]"'

The *** are because the secret itself is arm64 and that's being masked (note that the masks are case sensitive, which is why we can still see ARM64).

Right now, the: Linux ARM64 job shows:

Started 2m 0s ago
Requested labels: ["self-hosted", "linux", "ARM64"]
Job defined at: check-spelling/stack/.github/workflows/integration-tests.yml@refs/heads/integration-tests-in-forks
Waiting for a runner to pick up this job...

Sadly, when I cancel the job, that detail is eaten by GitHub.

mpilgrem

See my comment about quotaton marks causing problems with requesting a runner for Linux ARM64.

mpilgrem · 2022-09-08T16:10:45Z

.github/workflows/integration-tests.yml

+      run: |
+        echo "::set-output name=runners::$SELF_HOSTED_RUNNERS"
+        if echo "$SELF_HOSTED_RUNNERS" | grep -q 'arm64'; then
+          echo '::set-output name=arm64::"[\"self-hosted\", \"linux\", \"ARM64\"]"'


Are the quotation marks causing the problem with the hiatus at:

Requested labels: ["self-hosted", "linux", "ARM64"] Job defined at: commercialhaskell/stack/.github/workflows/integration-tests.yml@refs/pull/5856/merge Waiting for a runner to pick up this job...

The original was as follows (no quotation marks):

linux-arm64: name: Linux ARM64 runs-on: [self-hosted, linux, ARM64]

I'm not sure. I don't have a personal self-hosted runner (I have one at work that I could play w/).

In general, I'd expect fromJSON to do more or less the right thing. Since I think it's supposed to be a string or an array per:
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idruns-on

You can provide runs-on as a single string or as an array of strings. If you specify an array of strings, your workflow will run on a self-hosted runner whose labels match all of the specified runs-on values, if available. If you would like to run your workflow on multiple machines, use jobs.<job_id>.strategy.

I guessed that this was the right notation to make it happy.

Let's assume that I guessed wrong.

If it turns out I guessed wrong, I'll file at least one bug against this -- I'll probably start w/ the docs repo, although I feel that this is a bug in the impl...

I've made the change - lets see what happens ...

jsoref · 2022-09-08T16:23:33Z

It seems to do the right thing:
Linux ARM64

Started 1m 37s ago
Requested labels: [self-hosted, linux, ARM64]
Job defined at: check-spelling/stack/.github/workflows/integration-tests.yml@refs/heads/integration-tests-in-forks
Waiting for a runner to pick up this job...

jsoref added 4 commits September 8, 2022 10:32

Add self hosted runners guard

2a477c9

Only sign if key is available

2da3321

Creating a release requires write permissions

4bf9129

Switch to ncipollo/release-action

24d0f4d

jsoref force-pushed the integration-tests-in-forks branch from 8adc28c to 24d0f4d Compare September 8, 2022 14:35

jsoref commented Sep 8, 2022

View reviewed changes

mpilgrem reviewed Sep 8, 2022

View reviewed changes

jsoref added 2 commits September 8, 2022 13:13

Maybe this field should always be a string instead of sometimes an array

c86622b

move fromJSON to runs-on

5c9db84

jsoref force-pushed the integration-tests-in-forks branch from 3c3140c to 5c9db84 Compare September 8, 2022 17:13

Remove quotation marks from configuration.outputs.arm64

1305b24

jsoref mentioned this pull request Sep 8, 2022

fromJSON should include an example of an array for runs-on github/docs#20495

Closed

1 task

mpilgrem added 2 commits September 8, 2022 19:18

Try with minimal quotation marks in echo

ee14c11

Try with single quotes around strings in echo

ba70082

mpilgrem merged commit 3c3f5ef into commercialhaskell:master Sep 8, 2022

jsoref deleted the integration-tests-in-forks branch September 8, 2022 20:03

Integration tests in forks #5856

Integration tests in forks #5856

Uh oh!

Conversation

jsoref commented Sep 8, 2022

Uh oh!

mpilgrem commented Sep 8, 2022

Uh oh!

mpilgrem commented Sep 8, 2022

Uh oh!

mpilgrem commented Sep 8, 2022

Uh oh!

jsoref commented Sep 8, 2022

Uh oh!

jsoref commented Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mpilgrem commented Sep 8, 2022

Uh oh!

jsoref commented Sep 8, 2022

Uh oh!

mpilgrem commented Sep 8, 2022

Uh oh!

jsoref commented Sep 8, 2022

Uh oh!

jsoref Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

jsoref commented Sep 8, 2022

Uh oh!

mpilgrem left a comment

Choose a reason for hiding this comment

Uh oh!

mpilgrem Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

jsoref Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

mpilgrem Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

jsoref commented Sep 8, 2022

Uh oh!

Uh oh!

jsoref commented Sep 8, 2022 •

edited

Loading