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 changelog checker #4387

Merged
merged 3 commits into from
Nov 23, 2021
Merged

Conversation

agraul
Copy link
Member

@agraul agraul commented Oct 21, 2021

What does this PR change?

Use get-changed-files action, it also works on merged pull requests. Before, $files_modified was empty on merged pull requests and the workflow failed. Also simplified the ruby script while I was at it.

GUI diff

No diff.

  • DONE

Documentation

  • No documentation needed: github actions only

  • DONE

Test coverage

  • No tests: github actions only

GitHub is a bit slow for me today, I want to do another manual test in my fork before ticking "done".

  • DONE

Links

Couldn't find any issue.

  • DONE

Changelogs

If you don't need a changelog check, please mark this checkbox:

  • No changelog needed

Re-run a test

If you need to re-run a test, please mark the related checkbox, it will be unchecked automatically once it has re-run:

  • Re-run test "changelog_test"
  • Re-run test "backend_unittests_pgsql"
  • Re-run test "java_pgsql_tests"
  • Re-run test "schema_migration_test_oracle"
  • Re-run test "schema_migration_test_pgsql"
  • Re-run test "susemanager_unittests"
  • Re-run test "javascript_lint"
  • Re-run test "spacecmd_unittests"

This action also works on merged pull requests. Before, $files_modified
was empty on merged pull requests and the workflow failed
.github/scripts/changelog.rb Outdated Show resolved Hide resolved
Only .changes files are passed to this script by the get-changed-files
action.
@Etheryte
Copy link
Member

Out of curiosity, does get-changed-files also work for the pull_request_target hook (as opposed to the pull_request one we have right now)? If it does, we could also fix the problem that right now the bot can't comment on PRs when needed due to permissions. This is something that I tried to fix in #4368 and reverted in #4374 exactly because of issues with getting the changed files list.

@agraul
Copy link
Member Author

agraul commented Oct 22, 2021

Out of curiosity, does get-changed-files also work for the pull_request_target hook (as opposed to the pull_request one we have right now)?

Yes, this one does:

This project is a fork of https://github.com/jitterbit/get-changed-files, which supports pull_request_target, allow to filter files using regular expressions and removes the ahead check.

https://github.com/jitterbit/get-changed-files seems unmaintained, the PR that adds pull_request_target support is open for close to a year now. Hence this uses Ana's fork.

If it does, we could also fix the problem that right now the bot can't comment on PRs when needed due to permissions. This is something that I tried to fix in #4368 and reverted in #4374 exactly because of issues with getting the changed files list.

I'll s/pull_request/pull_request_target/ in the workflow yaml

The automation to create comments doesn't work on the pull_request hook
for forks due to permissions. According to the docs, the right hook to
use is pull_request_target instead.

Co-authored-by: Etheryte <[email protected]>
@Etheryte
Copy link
Member

Nice, thanks a lot for looking into this as well, this looks very good. 👍

Copy link
Contributor

@cbosdo cbosdo left a comment

Choose a reason for hiding this comment

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

LGTM but I honestly have no clue about github actions... so not sure such a review is actually useful ;)

@agraul agraul added the merge-candidate Meaning it needs to be considered for merging when the master branch is frozen label Nov 23, 2021
@juliogonzalez juliogonzalez merged commit 2a64edf into uyuni-project:master Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-candidate Meaning it needs to be considered for merging when the master branch is frozen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants