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

Fix PR Metadata and Downstream issues #1466

Merged
merged 21 commits into from
Oct 20, 2023
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
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
16 changes: 16 additions & 0 deletions .github/actions/add-label/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
## Add Label

## Inputs
* `issue_number`: The issue number derived from the event payload. Default is `${{ github.event.number }}` then `${{ github.event.issue.number }}`. Action will fail if neither of these is present. If you need to use this action on a push event (or any event not associated directly with an `issue` or `pull_request`, please see [gh-find-current-pr](https://github.com/jwalton/gh-find-current-pr))
Copy link
Contributor

Choose a reason for hiding this comment

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

The default value for issue_number is first set to ${{ github.event.number }} and then ${{ github.event.issue.number }}. However, the action will fail if neither of these is present. This could be a potential issue if the action is used in a context where these values are not available. Consider adding error handling or a more descriptive failure message to guide users in such scenarios.

* `label`: The label to add to the issue


## Usage:

```yaml
- name: Add Label
uses: ./.github/actions/add-label
with:
label: 'needs-go-generate-${{matrix.package}}'
issue-number: ${{github.event.number}}
```
trajan0x marked this conversation as resolved.
Show resolved Hide resolved
43 changes: 43 additions & 0 deletions .github/actions/add-label/action.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
name: Add Label
description: Add a label to the issue or pull request
inputs:
issue_number:
description: 'The issue number to associate with. Defaults to the current issue or pull request number used to trigger the action.'
required: false
default: ''
label:
description: 'The label to add.'
required: true

runs:
using: 'composite'
steps:
# # TODO, dedupe w/ remove-label
- name: Resolve issue number
id: resolve_issue_number
run: |
if [[ -n '${{ inputs.issue_number }}' ]]; then
echo 'Using input issue number: ${{ inputs.issue_number }}'
echo '::set-output name=issue::${{ inputs.issue_number }}'
elif [[ -n '${{ github.event.number }}' ]]; then
echo 'Using event number: ${{ github.event.number }}'
echo '::set-output name=issue::${{ github.event.number }}'
elif [[ -n '${{ github.event.issue.number }}' ]]; then
echo 'Using event issue number: ${{ github.event.issue.number }}'
echo '::set-output name=issue::${{ github.event.issue.number }}'
else
echo 'Could not determine issue number'
exit 1
fi
shell: bash

- name: Add Label
uses: actions/github-script@v6
with:
script: |
github.rest.issues.addLabels({
issue_number: ${{ steps.resolve_issue_number.outputs.issue }},
owner: context.repo.owner,
repo: context.repo.repo,
labels: ['${{ inputs.label }}']
})
14 changes: 14 additions & 0 deletions .github/actions/remove-label/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
## Add Label

## Inputs
* `issue_number`: The issue number derived from the event payload. Default is `${{ github.event.number }}` then `${{ github.event.issue.number }}`. Action will fail if neither of these is present. If you need to use this action on a push event (or any event not associated directly with an `issue` or `pull_request`, please see [gh-find-current-pr](https://github.com/jwalton/gh-find-current-pr))
* `label`: The label to add to the issue
Copy link
Contributor

Choose a reason for hiding this comment

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

The description of the label input is incorrect. It says "The label to add to the issue", but this action is for removing labels, not adding them. Please correct the description to accurately reflect the input's purpose.

- * `label`: The label to add to the issue
+ * `label`: The label to remove from the issue



```yaml
- name: Remove Label
uses: ./.github/actions/remove-label
with:
label: 'needs-go-generate-${{matrix.package}}'
issue-number: ${{github.event.number}}
```
43 changes: 43 additions & 0 deletions .github/actions/remove-label/action.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
name: Remove Label
description: Remove a label to the issue or pull request
inputs:
issue_number:
description: 'The issue number to associate with. Defaults to the current issue or pull request number used to trigger the action.'
required: false
default: ''
label:
description: 'The label to add.'
required: true

runs:
using: 'composite'
steps:
# TODO, dedupe w/ add-label
trajan0x marked this conversation as resolved.
Show resolved Hide resolved
- name: Resolve issue number
id: resolve_issue_number
run: |
if [[ -n '${{ inputs.issue_number }}' ]]; then
echo 'Using input issue number: ${{ inputs.issue_number }}'
echo '::set-output name=issue::${{ inputs.issue_number }}'
elif [[ -n '${{ github.event.number }}' ]]; then
echo 'Using event number: ${{ github.event.number }}'
echo '::set-output name=issue::${{ github.event.number }}'
elif [[ -n '${{ github.event.issue.number }}' ]]; then
echo 'Using event issue number: ${{ github.event.issue.number }}'
echo '::set-output name=issue::${{ github.event.issue.number }}'
else
echo 'Could not determine issue number'
exit 1
fi
Comment on lines +15 to +31
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 20, 2023

Choose a reason for hiding this comment

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

The code for resolving the issue number is duplicated in add-label and remove-label actions. Consider extracting this logic into a separate action or a script to avoid code duplication. This will improve maintainability and make the code easier to test and debug.

-     # TODO, dedupe w/ add-label
-     - name: Resolve issue number
-       id: resolve_issue_number
-       run: |
-         if [[ -n '${{ inputs.issue_number }}' ]]; then
-           echo 'Using input issue number: ${{ inputs.issue_number }}'
-           echo '::set-output name=issue::${{ inputs.issue_number }}'
-         elif [[ -n '${{ github.event.number }}' ]]; then
-           echo 'Using event number: ${{ github.event.number }}'
-           echo '::set-output name=issue::${{ github.event.number }}'
-         elif [[ -n '${{ github.event.issue.number }}' ]]; then
-           echo 'Using event issue number: ${{ github.event.issue.number }}'
-           echo '::set-output name=issue::${{ github.event.issue.number }}'
-         else
-           echo 'Could not determine issue number'
-           exit 1
-         fi
-       shell: bash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

already added a todo for this

Copy link
Contributor

Choose a reason for hiding this comment

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

@trajan0x, I understand that you've added a TODO comment to address this issue in the future. However, it's generally a good practice to avoid merging code that contains duplication, especially when the duplication is non-trivial and could be abstracted into a reusable component. This is in line with the DRY (Don't Repeat Yourself) principle, which is a fundamental concept in software development for reducing repetition of software patterns, replacing it with abstractions or using data normalization to avoid redundancy.

Here's a suggestion on how you could abstract the issue number resolution logic into a separate GitHub action:

name: Resolve Issue Number
description: Resolve the issue number from inputs or event context
inputs:
  issue_number:
    description: 'The issue number to associate with. Defaults to the current issue or pull request number used to trigger the action.'
    required: false
    default: ''
runs:
  using: 'composite'
  steps:
    - name: Resolve issue number
      id: resolve_issue_number
      run: |
        if [[ -n '${{ inputs.issue_number }}' ]]; then
          echo 'Using input issue number: ${{ inputs.issue_number }}'
          echo '::set-output name=issue::${{ inputs.issue_number }}'
        elif [[ -n '${{ github.event.number }}' ]]; then
          echo 'Using event number: ${{ github.event.number }}'
          echo '::set-output name=issue::${{ github.event.number }}'
        elif [[ -n '${{ github.event.issue.number }}' ]]; then
          echo 'Using event issue number: ${{ github.event.issue.number }}'
          echo '::set-output name=issue::${{ github.event.issue.number }}'
        else
          echo 'Could not determine issue number'
          exit 1
        fi
      shell: bash
outputs:
  issue:
    description: 'The resolved issue number'

Then, in your add-label and remove-label actions, you can use this new action like so:

- name: Resolve Issue Number
  id: resolve_issue_number
  uses: ./.github/actions/resolve-issue-number
  with:
    issue_number: ${{ inputs.issue_number }}

This way, you can keep your code DRY and maintainable.

shell: bash

- name: Remove Label
uses: actions/github-script@v6
with:
script: |
github.rest.issues.removeLabel({
issue_number: ${{ steps.resolve_issue_number.outputs.issue }},
owner: context.repo.owner,
repo: context.repo.repo,
name: ['${{ inputs.label }}']
})
trajan0x marked this conversation as resolved.
Show resolved Hide resolved
5 changes: 5 additions & 0 deletions .github/actions/setup-nodejs/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ runs:
shell: bash
id: nvmrc

- name: Setup Globals
shell: bash
run: |
echo "enableGlobalCache true" >> ~/.yarnrc

- uses: actions/setup-node@v3
with:
node-version: '${{ steps.nvmrc.outputs.NVMRC }}'
Expand Down
53 changes: 22 additions & 31 deletions .github/workflows/go.yml
Original file line number Diff line number Diff line change
Expand Up @@ -353,16 +353,15 @@ jobs:
needs: cancel-outdated
# currently, this matches the logic in the go generate check. If we ever add more checks that run on all packages, we should
# change this to run on those pushes
if: ${{ github.event_name != 'pull_request' && format('refs/heads/{0}', github.event.repository.default_branch) != github.ref }}
if: ${{ github.event_name != 'push' && format('refs/heads/{0}', github.event.repository.default_branch) != github.ref }}
outputs:
issue_number: ${{ steps.find_pr.outputs.pr }}
metadata: ${{ steps.metadata.outputs.METADATA }}
labels: ${{ steps.metadata.outputs.LABELS }}
steps:
- uses: jwalton/gh-find-current-pr@v1
id: find_pr
# TODO: https://stackoverflow.com/a/75429845 consider splitting w/ gql to reduce limit hit
- run: |
- name: Fetch Metadata for ${{github.event.number}}
run: |
# Fetch the metadata
metadata="$(gh api repos/$OWNER/$REPO_NAME/pulls/$PULL_REQUEST_NUMBER)"

Expand All @@ -380,15 +379,15 @@ jobs:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
OWNER: ${{ github.repository_owner }}
REPO_NAME: ${{ github.event.repository.name }}
PULL_REQUEST_NUMBER: ${{ steps.find_pr.outputs.pr }}
PULL_REQUEST_NUMBER: ${{github.event.number}}

# check if we need to rerun go generate as a result of solidity changes. Note, this will only run on solidity changes.
# TODO: consolidate w/ go change check. This will run twice on agents
check-generation-solidity:
name: Go Generate (Solidity Only)
runs-on: ubuntu-latest
needs: [changes, pr_metadata]
if: ${{ github.event_name != 'pull_request' && needs.changes.outputs.solidity_changes == 'true' }}
if: ${{ github.event_name != 'push' && needs.changes.outputs.solidity_changes == 'true' }}
strategy:
fail-fast: false
matrix:
Expand Down Expand Up @@ -472,48 +471,44 @@ jobs:
# TODO: this can run into a bit of a race condition if any other label is removed/added while this is run, look into fixing this by dispatching another workflow
- name: Add Label
if: ${{ !contains(fromJson(needs.pr_metadata.outputs.labels), format('needs-go-generate-{0}', matrix.package)) && steps.verify-changed-files.outputs.files_changed == 'true' }}
uses: andymckay/labeler@3a4296e9dcdf9576b0456050db78cfd34853f260
uses: ./.github/actions/add-label
with:
add-labels: 'needs-go-generate-${{matrix.package}}'
repo-token: ${{ secrets.GITHUB_TOKEN }}
issue-number: ${{ needs.pr_metadata.outputs.issue_number }}
label: 'needs-go-generate-${{matrix.package}}'

- name: Remove Label
if: ${{ contains(fromJson(needs.pr_metadata.outputs.labels), format('needs-go-generate-{0}', matrix.package)) && steps.verify-changed-files.outputs.files_changed != 'true' }}
uses: andymckay/labeler@3a4296e9dcdf9576b0456050db78cfd34853f260
uses: ./.github/actions/remove-label
with:
remove-labels: 'needs-go-generate-${{matrix.package}}'
repo-token: ${{ secrets.GITHUB_TOKEN }}
issue-number: ${{ needs.pr_metadata.outputs.issue_number }}
label: 'needs-go-generate-${{matrix.package}}'

remove-label-generation:
name: Remove Generate Label From Unused Jobs
runs-on: ubuntu-latest
needs: [changes, pr_metadata]
if: ${{ github.event_name != 'pull_request' && needs.changes.outputs.unchanged_package_count_deps > 0 && contains(needs.pr_metadata.outputs.labels, 'needs-go-generate') }}
if: ${{ github.event_name != 'push' && needs.changes.outputs.unchanged_package_count_deps > 0 && contains(needs.pr_metadata.outputs.labels, 'needs-go-generate') }}
strategy:
fail-fast: false
max-parallel: 1
matrix:
# only do on agents for now. Anything that relies on solidity in a package should do this
package: ${{ fromJSON(needs.changes.outputs.unchanged_deps) }}
steps:
# needed for remove label action
- uses: actions/checkout@v4
if: ${{ contains(fromJson(needs.pr_metadata.outputs.labels), format('needs-go-generate-{0}', matrix.package)) }}
with:
fetch-depth: 1
- name: Remove Label
if: ${{ contains(fromJson(needs.pr_metadata.outputs.labels), format('needs-go-generate-{0}', matrix.package)) }}
# labels can't be removed in parallel
uses: andymckay/labeler@3a4296e9dcdf9576b0456050db78cfd34853f260
uses: ./.github/actions/remove-label
with:
remove-labels: 'needs-go-generate-${{matrix.package}}'
repo-token: ${{ secrets.GITHUB_TOKEN }}
issue-number: ${{ needs.pr_metadata.outputs.issue_number }}

label: 'needs-go-generate-${{matrix.package}}'


check-generation:
name: Go Generate (Module Changes)
runs-on: ubuntu-latest
needs: [changes, pr_metadata]
if: ${{ github.event_name != 'pull_request' && needs.changes.outputs.package_count_deps > 0 }}
if: ${{ github.event_name != 'push' && needs.changes.outputs.package_count_deps > 0 }}
strategy:
fail-fast: false
matrix:
Expand Down Expand Up @@ -635,16 +630,12 @@ jobs:
# TODO: this can run into a bit of a race condition if any other label is removed/added while this is run, look into fixing this by dispatching another workflow
- name: Add Label
if: ${{ !contains(fromJson(needs.pr_metadata.outputs.labels), format('needs-go-generate-{0}', matrix.package)) && steps.verify-changed-files.outputs.files_changed == 'true' }}
uses: andymckay/labeler@3a4296e9dcdf9576b0456050db78cfd34853f260
uses: ./.github/actions/add-label
with:
add-labels: 'needs-go-generate-${{matrix.package}}'
repo-token: ${{ secrets.GITHUB_TOKEN }}
issue-number: ${{ needs.pr_metadata.outputs.issue_number }}
label: 'needs-go-generate-${{matrix.package}}'

- name: Remove Label
if: ${{ contains(fromJson(needs.pr_metadata.outputs.labels), format('needs-go-generate-{0}', matrix.package)) && steps.verify-changed-files.outputs.files_changed != 'true' }}
uses: andymckay/labeler@3a4296e9dcdf9576b0456050db78cfd34853f260
uses: ./.github/actions/remove-label
with:
remove-labels: 'needs-go-generate-${{matrix.package}}'
repo-token: ${{ secrets.GITHUB_TOKEN }}
issue-number: ${{ needs.pr_metadata.outputs.issue_number }}
label: 'needs-go-generate-${{matrix.package}}'
16 changes: 6 additions & 10 deletions .github/workflows/packages.yml
Original file line number Diff line number Diff line change
Expand Up @@ -60,20 +60,16 @@ jobs:
yarn.lock

- name: Add Label
if: steps.verify-yarn-lock.outputs.files_changed == 'true'
uses: andymckay/labeler@3a4296e9dcdf9576b0456050db78cfd34853f260
if: ${{ steps.verify-yarn-lock.outputs.files_changed == 'true' && github.event_name != 'push' }}
uses: ./.github/actions/add-label
with:
add-labels: 'needs-yarn-install'
repo-token: ${{ secrets.GITHUB_TOKEN }}
issue-number: ${{ needs.issue_number.outputs.issue_number }}
label: 'needs-yarn-install'

- name: Remove Label
if: steps.verify-yarn-lock.outputs.files_changed != 'true'
uses: andymckay/labeler@3a4296e9dcdf9576b0456050db78cfd34853f260
if: ${{ steps.verify-yarn-lock.outputs.files_changed != 'true' && github.event_name != 'push' }}
uses: ./.github/actions/add-label
with:
remove-labels: 'needs-yarn-install'
repo-token: ${{ secrets.GITHUB_TOKEN }}
issue-number: ${{ needs.issue_number.outputs.issue_number }}
label: 'needs-yarn-install'


- name: Run tests # Run tests of all packages
Expand Down