Skip to content

Option to update the PR comment – improvements #42

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Option to update the PR comment – improvements #42

wants to merge 1 commit into from

Conversation

bartosz347
Copy link
Contributor

@bartosz347 bartosz347 commented Nov 23, 2022 •
edited
Loading

I've rebased the PR #28 created by @oacik and applied some improvements to properly handle minimum-coverage check.

I've tested the changes in a test repository – bartosz347/github-actions-flutter-workflows#1. The build is failing intentionally – to test minimum coverage requirement. I've tested the following scenarios:

  1. Current version, minimum coverage requirement satisfied
  2. Current version, minimum coverage requirement not satisfied
  3. Updated version, minimum coverage requirement satisfied
  4. Updated version, minimum coverage requirement not satisfied
  5. Updated version, minimum coverage requirement satisfied, update-comment: false (regression)

In both 3. and 4. the comment bartosz347/github-actions-flutter-workflows#1 (comment) has been updated. In 5. new comment has been created.

If there's any better way to contribute to the previous PR, please let me know. Thanks @oacik for your work!

@bartosz347 bartosz347 mentioned this pull request Nov 23, 2022
Closed
@bartosz347 bartosz347 force-pushed the option-to-update-pr-comment_fixes branch from ef498d7 to 26ab5bf Compare November 24, 2022 13:36
@zgosalvez
Copy link
Owner

Hi @bartosz347! Curious why you didn't simply contribute to the original PR #28?

@bartosz347
Copy link
Contributor Author

Hey @zgosalvez, that's what I've wanted to do at first, but I think I can't contribute to the existing PR – I'd have to push to its author's repository (I don't have RW access). Only maintainers are allowed to add new commits. Or maybe I'm wrong – please tell me how can I do this :)

@zgosalvez
Copy link
Owner

Oh yeah, I forgot. 😅 Well, I did take a look at your PR and it appears to be somewhat unrelated to #28, right? #28 is simply about providing the option to update a comment instead of creating a new one. Whereas your PR here is an improvement on minimum coverage, correct?

@bartosz347
Copy link
Contributor Author

Not exactly, #28 has a return statement which omits isFailure check (see https://github.com/zgosalvez/github-actions-report-lcov/pull/28/files#r935143815) – I believe minimum code coverage condition does not work properly in #28.

@bartosz347 bartosz347 force-pushed the option-to-update-pr-comment_fixes branch 2 times, most recently from df3091c to 767e21c Compare December 5, 2022 18:36
@bartosz347 bartosz347 force-pushed the option-to-update-pr-comment_fixes branch from 767e21c to 4956737 Compare January 2, 2023 17:46
@bartosz347 bartosz347 force-pushed the option-to-update-pr-comment_fixes branch from 4956737 to 84680f0 Compare January 12, 2023 20:25
@bartosz347
Copy link
Contributor Author

Hey @zgosalvez looks like something's wrong with Build & Test pipeline, it can't check out code from fork. Would you be able to fix this and merge my PR?

@bartosz347 bartosz347 force-pushed the option-to-update-pr-comment_fixes branch from 84680f0 to bc0168d Compare January 23, 2023 16:19
@bartosz347 bartosz347 mentioned this pull request Jan 28, 2023
Closed
@kdawgwilk
Copy link

Would love to see this addition and fixes for v3!

@zgosalvez zgosalvez mentioned this pull request Feb 19, 2023
Merged
@zgosalvez
Copy link
Owner

Addressed in #85 🚀

@zgosalvez zgosalvez closed this Feb 19, 2023
@bartosz347 bartosz347 deleted the option-to-update-pr-comment_fixes branch February 19, 2023 18:51
pavelsaman pushed a commit to pavelsaman/github-actions-report-lcov that referenced this pull request Jun 25, 2024
@dependabot
deps: bump @biomejs/biome from 1.8.1 to 1.8.2 (zgosalvez#42)
24c8c1f 
Bumps [@biomejs/biome](https://github.com/biomejs/biome/tree/HEAD/packages/@biomejs/biome) from 1.8.1 to 1.8.2.
- [Release notes](https://github.com/biomejs/biome/releases)
- [Changelog](https://github.com/biomejs/biome/blob/main/CHANGELOG.md)
- [Commits](https://github.com/biomejs/biome/commits/cli/v1.8.2/packages/@biomejs/biome)

---
updated-dependencies:
- dependency-name: "@biomejs/biome"
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bartosz347 @zgosalvez @kdawgwilk @oacik