Skip to content

[nrf noup] ci: Fix manifest-pr-skip #2086

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

Closed
wants to merge 1 commit into from
Closed

[nrf noup] ci: Fix manifest-pr-skip #2086

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions .github/workflows/manifest-PR.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ jobs:
runs-on: ubuntu-latest
steps:
- name: handle manifest PR
if: >
!(contains(github.event.pull_request.title, 'manifest-pr-skip') ||
contains(github.event.pull_request.body, 'manifest-pr-skip'))
Comment on lines +14 to +16
Copy link
Contributor Author

@57300 57300 Oct 9, 2024

Choose a reason for hiding this comment

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

Hopefully it works, but I don't know how to verify it in advance.

Copy link
Contributor

@rugeGerritsen rugeGerritsen Oct 9, 2024

Choose a reason for hiding this comment

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

@57300 . I believe this should rather be fixed in the action itself, so that it also works for other users of this action. @karhama , do you agree?

Copy link
Contributor

@karhama karhama Oct 9, 2024

Choose a reason for hiding this comment

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

Yes I would recommend changing action itself if cancelled status is causing problems. However I did not fully understand in which case cancelled becomes the problem? Idea in action side was roughly : success == manifest PR created, failure == failed to create manifest PR (when it should have been created) and cancelled == "neutral status" i.e. no need to create manifest PR.

Copy link
Contributor Author

@57300 57300 Oct 9, 2024

Choose a reason for hiding this comment

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

Updating the action itself sounds reasonable. I'm just unsure what's the best way to do it, seems to me like it could make the code more convoluted.

The only problem with the cancel is that it makes the PR red, which is misleading. It also shows up when filtering PRs by "status:failure". I think a code owner would still be able to merge it though, so I guess it's minor.

uses: nrfconnect/action-manifest-pr@main
with:
token: ${{ secrets.NCS_GITHUB_TOKEN }}
Expand Down
Loading