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.

Sign up for GitHub

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

Jump to bottom

chore(.github): Document and explain CI override directives #10318

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
+37 −20
Open

chore(.github): Document and explain CI override directives #10318

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cb9dd6b
chore(.github): Update stale references
gibson042 Oct 22, 2024
d5e97a3
chore(.github): Better document CI consumption of override directives…
gibson042 Oct 22, 2024
bc181ec
chore(.github): Put CI override instructions in the PR template
gibson042 Oct 22, 2024
562fcef
chore(.github): Remove German Capitalization of "issue" in the PR tem…
gibson042 Oct 22, 2024
06dbb61
fixup! chore(.github): Better document CI consumption of override dir…
gibson042 Oct 23, 2024
e7b054c
fixup! chore(.github): Put CI override instructions in the PR template
gibson042 Oct 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion .github/PULL_REQUEST_TEMPLATE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,18 @@
v ✰ Thanks for creating a PR! ✰
☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -->

<!-- Most PRs should close a specific Issue. All PRs should at least reference one or more Issues. Edit and/or delete the following lines as appropriate (note: you don't need both `refs` and `closes` for the same one): -->
<!-- Most PRs should close a specific issue. All PRs should at least reference one or more issues. Edit and/or delete the following lines as appropriate (note: you don't need both `refs` and `closes` for the same one): -->

closes: #XXXX
refs: #XXXX

<!-- Integration testing doesn't run for every PR, but can be opted into by adding label 'force:integration', and can be customized to use non-default external references by including lines here that **start** with leading-`#` directives:
Copy link
Member

Choose a reason for hiding this comment

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

This is helpful. How can we ensure that it's updated when the truth of this changes?

Maybe if each branch override config has a reference to this file? Alternately we could adopt a scheme like #integrate: <repo> <branch>

Not asking for that work just exploring options

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we need process here; changes to the list of directives are rare and the template is implicitly reviewed roughly once per PR, so I have confidence that it would catch up in short order. But if we do DRY out something like .github/actions/read-pr-directive, I could see adding a link from its description to this comment in the template.

Copy link
Member

Choose a reason for hiding this comment

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

Integration testing does run for every PR, but only once the PR is labelled to merge. force:integration allows running integration tests for every PR push.

Copy link
Member

Choose a reason for hiding this comment

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

Arguably the external references should be removed before merging the PR to verify that the integration with the default branch still works (our logic is not sufficiently sophisticated to ignore these directives at the time of merge, plus it's possible we might someday do synchronized merges across repos which requires one side to keep the directive through merging)

* (https://github.com/Agoric/documentation) #documentation-branch: $BRANCH_NAME
* (https://github.com/endojs/endo) #endo-branch: $BRANCH_NAME
* (https://github.com/Agoric/dapp-offer-up) #getting-started-branch: $BRANCH_NAME
* (https://github.com/Agoric/testnet-load-generator) #loadgen-branch: $BRANCH_NAME
-->

## Description
<!-- Add a description of the changes that this PR introduces and the files that are the most critical to review. -->

Expand Down
17 changes: 9 additions & 8 deletions .github/actions/restore-node/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,11 @@ runs:
persist-credentials: false
path: ${{ inputs.path }}

# Select a branch on Endo to test against by adding text to the body of the
# pull request. For example: #endo-branch: some-pr-branch
# The default is 'NOPE' to indicate not to check out Endo, just use
# the published NPM packages.
# Select an endo against which to test, defaulting to 'NOPE' for use of
# internally-referenced NPM packages but allowing overrides in the pull
# request description for referencing a branch of the
# [endo repository](https://github.com/endojs/endo) using lines like
# `#endo-branch: rank-strings-by-codepoint`
- name: Get the appropriate Endo branch
id: endo-branch
uses: actions/github-script@v7
Expand All @@ -53,8 +54,8 @@ runs:
script: |-
let branch = 'NOPE';
if (${{ inputs.ignore-endo-branch }}) {
// Skip endo branch
} else if (context.eventName === 'schedule') {
// Skip any override directive.
} else if (context.eventName === 'schedule') {
branch = 'master';
} else if (context.payload.pull_request) {
const { body } = context.payload.pull_request;
Expand All @@ -64,7 +65,7 @@ runs:
branch = result[1];
}
}
console.log('Endo override branch: ' + branch);
console.log('endo override branch: ' + branch);
gibson042 marked this conversation as resolved.
Show resolved Hide resolved
return branch;

- name: merge endo integration branch
Expand All @@ -85,7 +86,7 @@ runs:
if: steps.endo-branch.outputs.result != 'NOPE'
uses: actions/checkout@v4
with:
repository: agoric/endo
repository: endojs/endo
mhofman marked this conversation as resolved.
Show resolved Hide resolved
path: ./replacement-endo
ref: ${{ steps.endo-branch.outputs.result }}
clean: 'false'
Expand Down
21 changes: 13 additions & 8 deletions .github/workflows/integration.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,10 @@ jobs:
pre_check:
uses: ./.github/workflows/pre-check-integration.yml

# This job is meant to emulate what developers working with the Agoric platform will experience
# It should be kept in sync with https://agoric.com/documentation/getting-started/
# This job is meant to emulate what developers working with the Agoric
# platform will experience.
# It should be kept in sync with the "getting started" workflow at
# https://docs.agoric.com/guides/getting-started/
getting-started:
needs: pre_check
if: needs.pre_check.outputs.should_run == 'true'
Expand All @@ -60,9 +62,11 @@ jobs:
node-version: 18.18
keep-endo: 'true'

# Select a branch on dapp to test against by adding text to the body of the
# pull request. For example: #getting-started-branch: zoe-release-0.7.0
# The default is 'main'
# Select a branch of the
# [default dapp repository](https://github.com/Agoric/dapp-offer-up) (cf.
# packages/agoric-cli/src/main.js) against which to test, defaulting to
# 'main' but allowing overrides in the pull request description using
# lines like `#getting-started-branch: zoe-release-0.7.0`
- name: Get the appropriate dapp branch
id: get-branch
uses: actions/github-script@v7
Expand Down Expand Up @@ -138,9 +142,10 @@ jobs:
# the chances the content of snapshots may deviate between validators
xsnap-random-init: '1'

# Select a branch on loadgen to test against by adding text to the body of the
# pull request. For example: #loadgen-branch: user-123-update-foo
# The default is 'main'
# Select a branch of the
# [load generator dapp repository](https://github.com/Agoric/testnet-load-generator)
# to use, defaulting to 'main' but allowing overrides in the pull request
# description using lines like `#loadgen-branch: ceelab`
- name: Get the appropriate loadgen branch
id: get-loadgen-branch
uses: actions/github-script@v7
Expand Down
10 changes: 6 additions & 4 deletions .github/workflows/test-documentation.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,11 @@ jobs:
node-version: ${{ matrix.node-version }}
path: ./agoric-sdk

# Select a branch on dapp to test against by adding text to the body of the
# pull request. For example: #dapp-encouragement-branch: zoe-release-0.7.0
# The default is 'main'
# Select a branch of the
# [documentation repository](https://github.com/Agoric/documentation)
# against which to test, defaulting to 'main' but allowing overrides in
# the pull request description using lines like
# `#documentation-branch: node-22`
- name: Get the appropriate dapp branch
id: get-branch
uses: actions/github-script@v7
Expand All @@ -44,7 +46,7 @@ jobs:
branch = result[1];
}
}
console.log(branch);
console.log('documentation branch: ' + branch);
return branch;

- name: Check out dapp
Expand Down
Loading