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

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

gibson042
Copy link
Member

Description

Updates inline comments in GitHub actions files to explain CI override directives (#endo-branch, #getting-started-branch, etc.) and adds references to them in the PR template to reduce our dependence upon oral tradition.

Also, although out of scope for this PR and probably not needed, we might consider a future change to pull out a helper for consumption of such directives:

.github/**/*.yml
       - name: Get the appropriate dapp branch
         id: get-branch
-        uses: actions/github-script@v7
+        uses: ./.github/actions/read-pr-directive
         with:
-          result-encoding: string
-          script: |
-            let branch = 'main';
-            if (context.payload.pull_request) {
-              const { body } = context.payload.pull_request;
-              const regex = /^\#getting-started-branch:\s+(\S+)/m;
-              const result = regex.exec(body);
-              if (result) {
-                branch = result[1];
-              }
-            }
-            console.log('getting-started dapp branch: ' + branch);
-            return branch;
+          tag: '#getting-started-branch'
+          default-value: main
.github/actions/read-pr-directive
name: Read PR Directive
description: 'Read the value of a `#`-prefixed directive from a PR description'

inputs:
  tag:
    description: 'The directive tag to read (must start with `#`)'
    required: true
  default-value:
    description: 'The value to return in the absence of any overriding directive'
    required: true

outputs:
  result:
    description: >-
      The value of the last overriding directive, or the default value if the
      context is not a pull request or there is no such directive
    value: ${{ steps.script.outputs.result }}

runs:
  using: composite
  steps:
    - id: script
      uses: actions/github-script@v7
      env:
        TAG: ${{ inputs.tag }}
        DEFAULT_VALUE: ${{ inputs.default-value }}
      with:
        result-encoding: string
        script: |-
          const q = JSON.stringify;
          const { TAG, DEFAULT_VALUE } = process.env;
          if (!TAG.startsWith('#')) {
            throw Error(`malformed directive tag ${q(TAG)}`);
          }
          if (!context.payload.pull_request) return DEFAULT_VALUE;

          const { body } = context.payload.pull_request;
          const directives = body.matchAll(/^(\#[\w-]+):\s+(\S+)/gm);
          const selections = directives.filter(m => m[1] === TAG).map(m => m[2]);
          const selection = selections.pop();
          for (const ignored of selections) {
            console.warn(`ignoring ${q(`${TAG}: ${ignored}`)}`);
          }
          console.log(`${TAG}: ${selection}`);
          return selection || DEFAULT_VALUE;

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

☑️

Testing Considerations

n/a

Upgrade Considerations

n/a

@gibson042 gibson042 requested a review from a team as a code owner October 23, 2024 00:17

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)

.github/actions/restore-node/action.yml Outdated Show resolved Hide resolved
Copy link

cloudflare-workers-and-pages bot commented Oct 23, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: e7b054c
Status: ✅  Deploy successful!
Preview URL: https://4d17587f.agoric-sdk.pages.dev
Branch Preview URL: https://gibson-2024-10-ci-documentat.agoric-sdk.pages.dev

View logs


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.

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.


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.

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)

.github/actions/restore-node/action.yml Show resolved Hide resolved
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

seems ok to me

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.

4 participants