-
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
Closed
Closed
Patch for CI #2996
Changes from 48 commits
Commits
Show all changes
58 commits
Select commit
Hold shift + click to select a range
7f38b32
Change Added
Sharpz7 96519db
Virtual CI check addition
Sharpz7 ecef7cf
Adding debug
Sharpz7 bf65641
More debugging
Sharpz7 341dc13
More testing
Sharpz7 83800b9
Added Docker Compose Logs
Sharpz7 a819fb2
Fix
Sharpz7 52bd0db
More changes
Sharpz7 1001c7b
Extending Pulsar wait time
Sharpz7 4cfbd65
Merge branch 'master' into patch-required-checks
Sharpz7 17846b2
Try this.
Sharpz7 2917ece
Update test.yml
Sharpz7 508da8e
Update test.yml
Sharpz7 b3afffd
Update test.yml
Sharpz7 a897304
Debug checks
Sharpz7 ce91066
More debugging
Sharpz7 32562cd
Added debugg
Sharpz7 8a7268c
Merge branch 'master' into patch-required-checks
Sharpz7 7e1be9d
Trying fixes
Sharpz7 cc1f033
Removed url
Sharpz7 9069c43
Update ci.go
Sharpz7 01644af
Removed frequency in location it is not needed
Sharpz7 7f598bb
Merged jgiannuzzi/disk-space
Sharpz7 456d782
Trying new cache technique
Sharpz7 5082276
Bugfix
Sharpz7 1e6c131
Bugfix
Sharpz7 497f3d5
More bugfixes
Sharpz7 2fd8791
Another fix
Sharpz7 75a9454
Adding other cache changes
Sharpz7 349118c
Trying new cache
Sharpz7 414356e
Merge branch 'master' into patch-required-checks
Sharpz7 68d8992
Merge branch 'master' into patch-required-checks
Sharpz7 ed27e23
Some cache fixes
Sharpz7 4a4d536
Merge branch 'master' into patch-required-checks
Sharpz7 97cf30e
Bugfix
Sharpz7 512d323
New cache time
Sharpz7 81d1f4d
Merge https://github.com/armadaproject/armada into patch-required-checks
Sharpz7 8951815
Changed times
Sharpz7 6c4dd1e
Pre-Final Commit
Sharpz7 9db4190
Need to create cache
Sharpz7 b31cb2e
Commenting Out Cache
Sharpz7 a37a930
Should let me test
Sharpz7 ef0230f
Added design doc
Sharpz7 22bcd9d
A few more changes
Sharpz7 20d4261
Added a few more changes
Sharpz7 ad47bf5
Reverted fork only changes
Sharpz7 ea7c31b
Added CI Doc
Sharpz7 72a846a
Update build.yml
Sharpz7 85d3be1
Trying new change
Sharpz7 9f62931
Merge branch 'master' into patch-required-checks
Sharpz7 d393304
Added more changes
Sharpz7 22a9c7d
Merge branch 'patch-required-checks' of https://github.com/Sharpz7/ar…
Sharpz7 ab7ff8d
Our changes
Sharpz7 40a7618
Added some docs changes and a change to python
Sharpz7 d68fc9f
Added airflow fix
Sharpz7 fd689eb
Added doc change
Sharpz7 1123a43
Merge branch 'master' into patch-required-checks
Sharpz7 8a20780
Added debug step
Sharpz7 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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; | ||
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(); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -52,10 +52,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 | ||
/home/runner/go/bin | ||
key: ${{ runner.os }}-gocache-${{ hashFiles('**/go.sum') }} | ||
restore-keys: | | ||
${{ runner.os }}-gocache- | ||
|
||
- name: Set up Docker Buildx | ||
id: buildx | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,2 @@ | ||
EXECUTOR_UPDATE_INTERVAL="1s" | ||
ARMADA_CORSALLOWEDORIGINS=="http://localhost:3000,http://localhost:8089,http://localhost:10000,http://example.com:10000,http://example.com:8089" | ||
ARMADA_CORSALLOWEDORIGINS=="http://localhost:3000,http://localhost:8089,http://localhost:10000,http://example.com:10000,http://example.com:8089" | ||
ARMADA_SCHEDULING_EXECUTORUPDATEFREQUENCY: "1s" |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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 comment
The 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 comment
The 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 comment
The 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 comment
The 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 comment
The 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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
You are reading my mind (with GR/actions)!