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: file issue if sigstore test fails #538

Merged
merged 1 commit into from
Mar 17, 2023

Conversation

lukpueh
Copy link
Member

@lukpueh lukpueh commented Mar 16, 2023

Unfortunately, we cannot run the sigstore test workflow on PR open because it requires 'id-token: write'-permissions, which is not available in that context.

To still detect if a code change breaks the sigstore test, we run it after the fact when the PR gets merged, and submit an issue in case.

This strategy is copied from test-kms.yml.

Unfortunately, we cannot run the sigstore test workflow on PR open
because it requires 'id-token: write'-permissions, which is not
available in that context.

To still detect if a code change breaks the sigstore test, we run it
after the fact when the PR gets merged, and submit an issue in
case.

This strategy is copied from test-kms.yml.

Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
@@ -11,6 +11,7 @@ permissions: {}
jobs:
test-sigstore:
runs-on: ubuntu-latest
if: github.repository_owner == 'secure-systems-lab' # only run upstream
Copy link
Member Author

Choose a reason for hiding this comment

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

@jku: I think we want this line in the kms workflow as well, even more than here actually. Because kms just fails in a fork that does not have GCP configured. Usually it even fails two times in a fork, once for the missing GCP config, and once for trying to submit an issue, which are disabled by default. It's not a big deal, but adds an ❌

Sigstore tests should pass in a fork, I just restricted the workflow to not unnecessarily spam sigstore certificate logs.

"https://github.com/" + repo + "/actions/runs/" + context.runId + ")"
})
console.log("New issue created.")
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is an almost exact copy of the "file an issue" step from test-kms.yml, which, in turn looks a lot like that step in check-upstream-ed25519.yml.

Unfortunately, I couldn't find a low-profile way of re-using the code. Composite actions requires a separate repo and a reusable workflow spins up a dedicated runner, which both seems overkill for a dozen lines of GitHub script.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As an aside: I found out recently that apparently the gh tool is installed on the actions builders: I'm not a heavy user of that tool but... I'm pretty sure it could be used for this and that the result might be simpler than the javascript here

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice!

Comment on lines +42 to +60
uses: actions/github-script@98814c53be79b1d30f795b907e553d8679345975
with:
script: |
const repo = context.repo.owner + "/" + context.repo.repo
const issues = await github.rest.search.issuesAndPullRequests({
q: "Sigstore+tests+failed+in:title+state:open+type:issue+repo:" + repo,
})
if (issues.data.total_count > 0) {
console.log("Issue open already, not creating.")
} else {
await github.rest.issues.create({
owner: context.repo.owner,
repo: context.repo.repo,
title: "Sigstore tests failed",
body: "Hey, it seems Sigstore tests have failed, please see - [workflow run](" +
"https://github.com/" + repo + "/actions/runs/" + context.runId + ")"
})
console.log("New issue created.")
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is what a qh version would like like. Haven't tried it in an action yet, only locally. I think it's cool, but I'm not sure if it is substantially easier to maintain than the JavaScript.

Suggested change
uses: actions/github-script@98814c53be79b1d30f795b907e553d8679345975
with:
script: |
const repo = context.repo.owner + "/" + context.repo.repo
const issues = await github.rest.search.issuesAndPullRequests({
q: "Sigstore+tests+failed+in:title+state:open+type:issue+repo:" + repo,
})
if (issues.data.total_count > 0) {
console.log("Issue open already, not creating.")
} else {
await github.rest.issues.create({
owner: context.repo.owner,
repo: context.repo.repo,
title: "Sigstore tests failed",
body: "Hey, it seems Sigstore tests have failed, please see - [workflow run](" +
"https://github.com/" + repo + "/actions/runs/" + context.runId + ")"
})
console.log("New issue created.")
}
run: |
title="Sigstore tests failed"
issue_count=$(gh issue list --search "${title} in:title is:issue is:open" \
--json id --jq length)
if [ "${issue_count}" == "0" ]
then
gh issue create \
--title "${title}" \
--body "Hey, it seems Sigstore tests have failed, \
please see - [workflow run](${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/actions/runs/${GITHUB_RUN_ID})."
else
echo "Issue open already, not creating."
fi

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, but we could save it in a script and re-use that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, maybe not worth it 🤷 your call.

You will need an env variable to forward the token to the tool (GH_TOKEN: ${{ github.token }} I think) so that makes it yet a little more complicated

Copy link
Collaborator

@jku jku left a comment

Choose a reason for hiding this comment

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

LGTM

@lukpueh lukpueh merged commit 19a7ada into secure-systems-lab:main Mar 17, 2023
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