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

feat: implement error notification as pr comment #124

Merged
merged 6 commits into from
Apr 10, 2024

Conversation

lampajr
Copy link
Member

@lampajr lampajr commented Apr 8, 2024

Thank you for submitting this pull request

fixes #123

Description

Implement an optional notification mechanism on the original pull request for error tracking.

If enabled, the tool will create a new comment in the original PR (the one you tried to backport) with some infos of the error that occurred.

This is going to introduce other two inputs:

  • enable error notification, disabled by default
  • (optional) comment body, with a custom {{error}} placeholder that can be replaced by the actual error.

How Has This Been Tested?

Checklist

  • Tests added if applicable.
  • Documentation updated if applicable.

Merge criteria:

  • The commits and have meaningful messages; the author will squash them after approval or will ask to merge with squash.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

Note: dist/cli/index.js and dist/gha/index.js are automatically generated by git hooks and gh workflows.

First time here?

This project follows git conventional commits pattern, therefore the commits should have the following format:

<type>(<optional scope>): <subject>
empty separator line
<optional body>
empty separator line
<optional footer>

Where the type must be one of [build, chore, ci, docs, feat, fix, perf, refactor, revert, style, test]

NOTE: if you are still in a work in progress branch and you want to push your changes remotely, consider adding --no-verify for both commit and push, e.g., git push origin <feat-branch> --no-verify - this could become useful to push changes where there are still tests failures. Once the pull request is ready, please amend the commit and force-push it to keep following the adopted git commit standard.

How to prepare for a new release?

There is no need to manually update package.json version and CHANGELOG.md information. This process has been automated in Prepare Release Github workflow.

Therefore whenever enough changes are merged into the main branch, one of the maintainers will trigger this workflow that will automatically update version and changelog based on the commits on the git tree.

More details can be found in package release section of the README.

Copy link
Contributor

github-actions bot commented Apr 8, 2024

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
89.67% (-1.62% 🔻)
495/552
🟢 Branches
86.24% (+0.05% 🔼)
188/218
🟢 Functions
87.4% (-1.03% 🔻)
111/127
🟢 Lines
89.53% (-1.7% 🔻)
479/535
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / runner-util.ts
100% 100% 100% 100%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / console-logger-service.ts
85.71% (-6.59% 🔻)
100%
80% (-8.89% 🔻)
85.71% (-6.59% 🔻)
🟢
... / github-client.ts
86.36% (-1.36% 🔻)
66.67% (-4.76% 🔻)
66.67% (+2.38% 🔼)
86.15% (-1.35% 🔻)
🟢
... / gitlab-client.ts
84.78% (-9.19% 🔻)
75% (-6.82% 🔻)
88% (-3.67% 🔻)
84.09% (-9.58% 🔻)

Test suite run success

187 tests passing in 17 suites.

Report generated by 🧪jest coverage report action from 8b0412b

@lampajr
Copy link
Member Author

lampajr commented Apr 8, 2024

Status update:

  • Added new cli argument --enable-err-notification to enable this new feature
  • Set default error message Backporting failed: {{error}} where {{error}} gets replaced by the actual exception msg

Open points:

  • I don't really like the default comment message I set right now, any feedback is welcome
  • Make the comment message configurable, I think this is a good thing but it can be done in a followup PR

TODOs:

  • Implement the Gitlab client
  • Update documentation
  • Try e2e / manual tests

@lampajr

This comment was marked as off-topic.

Copy link
Contributor

@earl-warren earl-warren left a comment

Choose a reason for hiding this comment

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

One tiny change otherwise looks great.

action.yml Outdated Show resolved Hide resolved
Co-authored-by: Earl Warren <[email protected]>
@lampajr
Copy link
Member Author

lampajr commented Apr 8, 2024

One tiny change otherwise looks great.

Thanks, copy&paste issue lol

I need to drop for today, but I plan to finalize this tomorrow 🚀

@lampajr
Copy link
Member Author

lampajr commented Apr 9, 2024

Open points:

* I don't really like the default comment message I set right now, any feedback is welcome

As first implementation let's go with Backporting failed: ${{error}}

* Make the comment message configurable, I think this is a good thing but it can be done in a followup PR

Will create separate issue to make the comment body configurable through templating

TODOs:

  • Implement the Gitlab client
  • Update documentation
  • Try e2e / manual tests

Still need to try some real examples, did not have time to setup ad hoc test but will do asap.

@lampajr lampajr marked this pull request as ready for review April 9, 2024 20:41
@lampajr
Copy link
Member Author

lampajr commented Apr 10, 2024

Ok I was able to test it with some real examples, e.g., lampajr/backporting-example#66 (comment)

With latest two commits:

  • Disable the comment if dry-run is enabled
  • Updated the default comment to "The backport to <TARGET_BRANCJ> failed. Check the latest run for more details."

I decided to remove the error from the comment as it could become very huge if the conflicts are many, e.g., lampajr/backporting-example#66 (comment)

@lampajr lampajr requested a review from earl-warren April 10, 2024 20:38
@lampajr
Copy link
Member Author

lampajr commented Apr 10, 2024

@earl-warren If you want to take a further look the PR is ready, otherwise I will go and merge!

PS: don't feel obligated, you already did a lot!!!

Copy link
Contributor

@earl-warren earl-warren left a comment

Choose a reason for hiding this comment

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

🎉

@lampajr lampajr merged commit 2bb7f73 into kiegroup:main Apr 10, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a comment to the backported PR on failure
2 participants