-
Notifications
You must be signed in to change notification settings - Fork 132
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
Patch for CI #2996
Patch for CI #2996
Changes from all commits
7f38b32
96519db
ecef7cf
bf65641
341dc13
83800b9
a819fb2
52bd0db
1001c7b
4cfbd65
17846b2
2917ece
508da8e
b3afffd
a897304
ce91066
32562cd
8a7268c
7e1be9d
cc1f033
9069c43
01644af
7f598bb
456d782
5082276
1e6c131
497f3d5
2fd8791
75a9454
349118c
414356e
68d8992
ed27e23
4a4d536
97cf30e
512d323
81d1f4d
8951815
6c4dd1e
9db4190
b31cb2e
a37a930
ef0230f
22bcd9d
20d4261
ad47bf5
ea7c31b
72a846a
85d3be1
9f62931
d393304
22a9c7d
ab7ff8d
40a7618
d68fc9f
fd689eb
1123a43
8a20780
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
# CI Documentation | ||
|
||
Please see [here](../docs/design/ci/oct-2023.md) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
name: Check required jobs | ||
|
||
# This workflow is triggered when a workflow run for the CI is completed. | ||
# It checks if the "All required checks done" job was actually successful | ||
# (and not just skipped) and creates a check run if that is the case. The | ||
# check run can be used to protect the main branch from being merged if the | ||
# CI is not passing. We need to use a GitHub app token to create the check | ||
# run because otherwise the check suite will be assigned to the first workflow | ||
# run for the CI, which might not be the latest one. See | ||
# https://github.com/orgs/community/discussions/24616#discussioncomment-6088422 | ||
# for more details. | ||
|
||
on: | ||
workflow_run: | ||
workflows: [CI] | ||
|
||
permissions: | ||
actions: read | ||
checks: write | ||
|
||
jobs: | ||
required-jobs: | ||
name: Check required jobs | ||
if: ${{ !github.event.repository.fork }} | ||
environment: create-check | ||
runs-on: ubuntu-latest | ||
steps: | ||
- name: Generate an app token | ||
id: app-token | ||
uses: actions/create-github-app-token@v1 | ||
with: | ||
app-id: ${{ secrets.APP_ID }} | ||
private-key: ${{ secrets.APP_PRIVATE_KEY }} | ||
|
||
- uses: actions/github-script@v6 | ||
with: | ||
github-token: ${{ steps.app-token.outputs.token }} | ||
script: | | ||
const ghaAppId = 15368; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this value be a secret? Also, const names are not concise. Would be good to rename to something more descriptive (ghaAppId, ghaName ,myAppId, myName) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think so. and since we use this check in other repos, I think we should keep it consistent between them in terms of naming There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jgiannuzzi do you know more about how to pick this ID? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh this is the public app ID of GitHub Actions 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If this is the case, then the best thing would be for this workflow to exist in a dedicated "reusable" repo, and then called (if such a thing is acceptable for Armada). What happens when we have 10 repos all having the same check-required.yml workflow, and something changes in one of them? Are we going to track down the 9 other repos that don't have the latest change and create a PR? @jgiannuzzi @Sharpz7 feel free to tell me i'm nitpicking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's a great point @pavlovic-ivan There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you agree, i think we should create an action out of this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure! You may want to first prototype it in some repo and have your forks of armada and fml use it. If it works well, we can probably create a GR/actions repo to collect all of our shared actions/pipelines, starting with this one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are reading my mind (with GR/actions)! |
||
const ghaName = 'All required checks done'; | ||
const myAppId = ${{ secrets.APP_ID }}; | ||
const myName = 'All required checks succeeded'; | ||
const owner = context.payload.repository.owner.login; | ||
const repo = context.payload.repository.name; | ||
const sha = context.payload.workflow_run.head_sha; | ||
|
||
core.info(`List GitHub Actions check runs for ${sha}.`) | ||
const { data: { check_runs: ghaChecks } } = await github.rest.checks.listForRef({ | ||
owner: owner, | ||
repo: repo, | ||
ref: sha, | ||
app_id: ghaAppId, | ||
check_name: ghaName, | ||
}); | ||
|
||
var newCheck = { | ||
owner: owner, | ||
repo: repo, | ||
name: myName, | ||
head_sha: sha, | ||
status: 'in_progress', | ||
started_at: context.payload.workflow_run.created_at, | ||
output: { | ||
title: 'Not all required checks succeeded', | ||
}, | ||
}; | ||
|
||
core.summary.addHeading('The following required checks have been considered:', 3); | ||
ghaChecks.forEach(check => { | ||
core.summary | ||
.addLink(check.name, check.html_url) | ||
.addCodeBlock(JSON.stringify(check, ['status', 'conclusion', 'started_at', 'completed_at'], 2), 'json'); | ||
|
||
if (check.status === 'completed' && check.conclusion === 'success') { | ||
newCheck.status = 'completed'; | ||
newCheck.conclusion = 'success'; | ||
newCheck.started_at = check.started_at; | ||
newCheck.completed_at = check.completed_at; | ||
newCheck.output.title = 'All required checks succeeded'; | ||
} else if (check.started_at > newCheck.started_at) { | ||
newCheck.started_at = check.started_at; | ||
} | ||
}); | ||
if (ghaChecks.length === 0) { | ||
core.summary.addRaw(`No check runs for ${sha} found.`); | ||
} | ||
newCheck.output.summary = core.summary.stringify(); | ||
await core.summary.write(); | ||
|
||
core.info(`Create own check run for ${sha}: ${JSON.stringify(newCheck, null, 2)}.`) | ||
const { data: { html_url } } = await github.rest.checks.create(newCheck); | ||
|
||
await core.summary | ||
.addHeading('Check run created:', 3) | ||
.addLink(myName, html_url) | ||
.addCodeBlock(JSON.stringify(newCheck, null, 2), 'json') | ||
.write(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,10 +54,23 @@ jobs: | |
- name: Fetch Git tags | ||
run: git fetch --force --tags | ||
|
||
- name: Setup Golang with Cache | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And here we have the inverse - does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above comment |
||
uses: magnetikonline/action-golang-cache@v4 | ||
- name: Set up Go with Cache | ||
uses: actions/setup-go@v4 | ||
with: | ||
go-version: "1.20" | ||
go-version: '1.20' | ||
cache: false | ||
|
||
# Check for cache | ||
- name: Cache Go modules | ||
uses: actions/cache/restore@v3 | ||
with: | ||
path: | | ||
/home/runner/.cache/go-build | ||
/home/runner/go/pkg/mod | ||
|
||
key: ${{ runner.os }}-gocache-${{ hashFiles('**/go.sum','**/tools.yaml') }} | ||
restore-keys: | | ||
${{ runner.os }}-gocache- | ||
|
||
- name: Set up Docker Buildx | ||
id: buildx | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the test, lint, and release workflows, we use the
magnetikonline/action-galng-cache
plugin, which functionally replaces the functionality of actions/setup-go and actions/cache/restore together - can that work here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - Now that I deleted the extra folder we cache, this can potentially change. But I do think having separate save and cache steps (Which is not possible with
magnetikonline/action-galng-cache
) is better?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the advantage of "separate save and cache steps" with using
actions/cache/restore
- I believe themagnetikonline
cache plugin automatically sets up the build and module cache paths as needed before, and persists those when the job is done.