test: run patch tests for optional modifications #3793
Conversation
Right now we don't know if it's broken, this tests will cover that issue
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…in permissions Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #3793 +/- ##
=======================================
Coverage 99.45% 99.45%
=======================================
Files 3 3
Lines 183 183
=======================================
Hits 182 182
Misses 1 1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
What do you think about running this workflow on pull requests? This does not run on pull requests as it is now, but we do not want to open an issue on a broken patch in a PR.
What is GITHUB_TOKEN secret value in PRs? If it just fails it's fine IMO and we can add on pull_requests.
I don't want to make the person who made the patch broken to be responsible to fixing the patch when they might not have understand how the patch should work, therefore I don't want to make the PR's CI red and blocking the merge process.
The same with |
|
So maintainer should test patches locally when someone opens a PR? |
Not really, no. I would prefer that the author (or maintainer) of the patch be notified when their patch can't be applied. |
|
I meant new PRs. Let's say user1 opens a PR which wants to add a patch, how should someone review it? |
|
Ah! You got a point. Let me fix that. |
|
There 😆 @aminvakil |
|
I think I'm multitasking badly right now and get everything mixed :) Sorry, I thought pull request was not activated on this workflow. |
| issue_body=("Failure during applying patches for $patch_dir:") | ||
|
|
||
| for patch_file in "$patch_dir"/*.patch; do | ||
| echo "Checking for existing issue for $patch_file" |
| # If the length of `issue_body` array is greater than 1, | ||
| # then we have a problem and we need to open an issue. | ||
| if [ "${#issue_body[@]}" -gt 1 ]; then | ||
| issue_exists=$(gh issue list --search "$issue_title in:title" --state open --json title --jq 'if length == 0 then empty end') | ||
| if [ -z "$issue_exists" ]; then | ||
| echo "Creating issue for $patch_dir" | ||
| gh issue create --title "$issue_title" --body "${issue_body[*]}" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
I'd say check for the issue first and if there's an existing issue skip the apply/test step too.
| fi | ||
|
|
||
| echo "::debug::Reset the repository state" | ||
| git reset --hard |
There was a problem hiding this comment.
These seem like cheap tests, only testing if the patch would apply cleanly. I think we can run these on PR for information and on master to create a new issue.
The "heavy" tests would be the ones which apply the patches and test if they actually work. This might be tricky.
At this point I'm wondering if we should spin off these patches into their own repos (for externals too) and find a way to apply these at install step easily. This would also offload the maintenance burden to whoever created the patches.
There was a problem hiding this comment.
These seem like cheap tests, only testing if the patch would apply cleanly. I think we can run these on PR for information and on master to create a new issue.
The "heavy" tests would be the ones which apply the patches and test if they actually work. This might be tricky.
It's not just tricky. If it's something like this external Kafka with auth + TLS support, you'd need to spawn an actual external Kafka with those configurations. Yet, I do think it is too heavy to do, and this minimal tests should work fine.
There was a problem hiding this comment.
If it's something like this external Kafka with auth + TLS support, you'd need to spawn an actual external Kafka with those configurations.
That's also why I want these in their own repos so those repo owners would be thinking about these, not us 😅
Right now we don't know if it's broken, this tests will cover that issue