Fix PR Metadata and Downstream issues #1466
Conversation
trajan0x
changed the title
This reverts commit b26a708.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1466 +/- ##
===================================================
+ Coverage 50.69210% 50.85025% +0.15814%
===================================================
Files 356 356
Lines 24274 24287 +13
Branches 267 268 +1
===================================================
+ Hits 12305 12350 +45
+ Misses 10775 10744 -31
+ Partials 1194 1193 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
…test anything because needs-go-generate doesn't run go mod tidy) This reverts commit 5ce64d2.
github-actions
bot
added
go
…t agents because of tob fixes
Co-authored-by: Trajan0x <trajan0x@users.noreply.github.com>
* try to fix yarn install time * yarn workflow mod * undo me (modified sdk-router yarn.lock, used to test #1466) * ignore me * Revert "ignore me" This reverts commit 6ae9e60. * fix spelling issue, try again * yarnrc fix * undo me * Revert "undo me" (Test warm cache) This reverts commit 032e1e7. --------- Co-authored-by: Trajan0x <trajan0x@users.noreply.github.com>
trajan0x
requested review from
aureliusbtc,
ChiTimesChi and
nautsimon
as code owners
WalkthroughThis pull request introduces custom GitHub Actions for adding and removing labels from issues or pull requests. It also refines the conditions for executing these actions and replaces third-party actions with custom ones. Additionally, it sets up global configuration for Yarn. Changes
TipsChat with CodeRabbit Bot (
|
trajan0x
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Files ignored due to filter (1)
- packages/sdk-router/yarn.lock
Files selected for processing (7)
- .github/actions/add-label/README.md (1 hunks)
- .github/actions/add-label/action.yml (1 hunks)
- .github/actions/remove-label/README.md (1 hunks)
- .github/actions/remove-label/action.yml (1 hunks)
- .github/actions/setup-nodejs/action.yml (1 hunks)
- .github/workflows/go.yml (4 hunks)
- .github/workflows/packages.yml (1 hunks)
Files skipped from review due to trivial changes (3)
- .github/actions/add-label/action.yml
- .github/actions/remove-label/README.md
- .github/actions/setup-nodejs/action.yml
Additional comments (Suppressed): 8
.github/workflows/packages.yml (2)
- 62-67: The "Add Label" action is correctly replaced with a custom action. However, the
issue_numberinput is missing, which is required to specify the issue or pull request to which the label should be added. Please verify if theissue_numberis being handled in the custom action or if it needs to be added here.with: label: 'needs-yarn-install' + issue_number: ${{ needs.issue_number.outputs.issue_number }}
- 68-72: The "Remove Label" action is incorrectly using the "Add Label" action. This should be replaced with the "Remove Label" action. Also, the
issue_numberinput is missing here as well.- uses: ./.github/actions/add-label + uses: ./.github/actions/remove-label with: label: 'needs-yarn-install' + issue_number: ${{ needs.issue_number.outputs.issue_number }}.github/workflows/go.yml (6)
- 356-356: The condition for running the job has been changed from
pull_requesttopush. Ensure that this change aligns with the intended workflow triggers.- 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 }}
363-366: The
gh apicommand is used to fetch metadata for the pull request. However, thegh apicommand requires authentication. Ensure that theGITHUB_TOKENis set in the environment variables to authenticate the GitHub CLI.390-390: The condition for running the job has been changed from
pull_requesttopush. Ensure that this change aligns with the intended workflow triggers.- if: ${{ github.event_name != 'pull_request' && needs.changes.outputs.solidity_changes == 'true' }} + if: ${{ github.event_name != 'push' && needs.changes.outputs.solidity_changes == 'true' }}
638-639: The deprecated third-party action
andymckay/labelerhas been replaced with custom actions for adding and removing labels. This is a good practice as it reduces dependencies on third-party actions which may become deprecated or introduce breaking changes.488-488: The condition for running the job has been changed from
pull_requesttopush. Ensure that this change aligns with the intended workflow triggers.- 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') }}
- 511-511: The condition for running the job has been changed from
pull_requesttopush. Ensure that this change aligns with the intended workflow triggers.- 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 }}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (1)
- .github/actions/remove-label/action.yml (1 hunks)
Additional comments (Suppressed): 1
.github/actions/remove-label/action.yml (1)
- 34-43: The
removeLabelfunction is expecting an array for thenameparameter. This was previously flagged as an issue but was clarified by the user with supporting documentation. No changes are needed here.
| # 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 |
There was a problem hiding this comment.
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: bashThere was a problem hiding this comment.
already added a todo for this
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Files selected for processing (2)
- .github/actions/add-label/README.md (1 hunks)
- .github/actions/remove-label/README.md (1 hunks)
Additional comments (Suppressed): 1
.github/actions/add-label/README.md (1)
- 12-18: The usage example is clear and concise, providing a good reference for how to use the action in a workflow. However, it would be beneficial to include an example of how to handle the action's failure, especially considering the potential issue mentioned above.
| This action adds a specified label to an issue or pull request. | ||
|
|
||
| ## 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)) |
There was a problem hiding this comment.
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.
| ## Remove Label | ||
|
|
||
| This action removes a specified label to an issue or pull request. |
There was a problem hiding this comment.
The description of the action is misleading. It says the action "removes a specified label to an issue or pull request", which implies that it's adding a label, not removing it. Please correct the description to accurately reflect the action's functionality.
- This action removes a specified label to an issue or pull request.
+ This action removes a specified label from an issue or pull request.|
|
||
| ## 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 |
There was a problem hiding this comment.
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* fix canto urls * list changed files for debugging * add label->remove label (Fix issue from #1466) --------- Co-authored-by: Trajan0x <trajan0x@users.noreply.github.com>
Description
This WIP PR aims to fix #1467 and #1434
To Do:
pr_metadatais even necessary. The whole point of find-current-pr is it allows us to associate a commit w/ a pull request. But unless we want to run the go generate stuff on push rather than pull request, this seems dumb. Maybe there's an argument for running on both if we comment on the commit instead, but I'm not sure this makes sense. A similiar concern was raised in Fix git-changes-action when module was unchanged #1399Update: I think this is completely fine, let's just run this on pull_requests. There's not a good way to get notified that's not extremely annoying on commits anyway.
andymckay/labelerwith changes in 39a9274 cccaf4a and 7da9e5c (Deprecated andymckay/labeler@3a4296e9dcdf9576b0456050db78cfd34853f260 is used #1467)packages.ymlworkflow (introduced in fix yarn immutable check #820) Done in Test/yarn fix #1469)After merging:
Optional/Future Work:
actions/setup-nodeinstall time. Dependencies are cached, but install still takes minutes. I've seen similar issues with similiar setups (can't getyarncache to restore effectively actions/setup-node#834 similiar, but yarn v3) , (github action cache not working actions/setup-node#819, this was blamed on branch issues except we have a cache hit??), (CI: speed up CI time by improving yarn install and caches (iteration 1 / >30%) strapi/strapi#16581, only supports yarn 3 :( ) (Attempted in 3767c90)Summary by CodeRabbit