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

Rewrite github action #1312

Merged
merged 1 commit into from
Apr 5, 2024

Conversation

zappolowski
Copy link
Member

@zappolowski zappolowski commented Mar 10, 2024

Yet another try of getting the pipelines to work. This continues/replaces #1303.

Notable changes:

  • update to use latest images from dunst-project/docker-images
  • updated the images for Debian bookworm and Alpine (see the other repository) to add some more missing dependencies
  • make test-coverage has issues running in parallel ⇒ -j should not be used
  • don't kill matrix jobs when one fails in order to see which targets are really broken
  • update actions where possible
  • misc-doxygen doesn't need to be a matrix, if it is a 1x1 matrix

TODO:

  • why generate coverage data for clang, when it's discarded anyhow (just gcc is uploaded); why upload coverage data for each distro? shouldn't one suffice?
  • codecov/codecov-action@v3 is outdated, but v4 doesn't work an all images
  • uploading coverage report fails on forks, this could be fixed in codecov/codecov-action@v4 (at least the description seems to imply this)
  • creating coverage reports on forks could be disabled altogether as it's done anyhow when the PR is created

@codecov-commenter
Copy link

codecov-commenter commented Mar 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.06%. Comparing base (a2af4a8) to head (03f5861).
Report is 6 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1312      +/-   ##
==========================================
- Coverage   65.52%   65.06%   -0.46%     
==========================================
  Files          48       48              
  Lines        7997     8173     +176     
==========================================
+ Hits         5240     5318      +78     
- Misses       2757     2855      +98     
Flag Coverage Δ
unittests 65.06% <ø> (-0.46%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bynect
Copy link
Member

bynect commented Mar 10, 2024

Seems good to me, however I am not sure what changes from #1303 (?)

  • why generate coverage data for clang, when it's discarded anyhow (just gcc is uploaded); why upload coverage data for each distro? shouldn't one suffice?

I second that.

@zappolowski
Copy link
Member Author

however I am not sure what changes from #1303 (?)

  • a different approach to fix this issue - instead of running as a different user, explicitly allow it using git config (as it's done in actions/checkout already)
  • alpine image is not excluded anymore
  • all tests are running - in Update GitHub actions #1303 DBus tests are deactivated

@bynect
Copy link
Member

bynect commented Mar 11, 2024

however I am not sure what changes from #1303 (?)

  • a different approach to fix this issue - instead of running as a different user, explicitly allow it using git config (as it's done in actions/checkout already)
  • alpine image is not excluded anymore
  • all tests are running - in Update GitHub actions #1303 DBus tests are deactivated

Thanks for the clarification. Is something still missing or will be able to fix ci by merging this one?

@zappolowski
Copy link
Member Author

From my side I'm okay with merging, maybe @bebehei wants to take another look as he originally started working on this.

@bebehei
Copy link
Member

bebehei commented Mar 12, 2024

I'll have alook tomorrow.

@bynect
Copy link
Member

bynect commented Mar 24, 2024

news?

@zappolowski
Copy link
Member Author

My suggestion: merge it in the current state (if @bebehei has no strong objections) and get the CI up and running again. This way we can merge some other PRs which are currently blocked by the broken CI.

Then I can iron out the missing parts.

@zappolowski zappolowski force-pushed the update-github-action branch 3 times, most recently from 476ad25 to 03f5861 Compare April 3, 2024 18:31
@zappolowski
Copy link
Member Author

I've reworked coverage creation a bit. Now, it's just created when all tests pass, only for gcc and only for the main repository (hopefully, haven't tested it yet). I've chosen to run that step on the fedora image as this is usually quite up-to-date, not a rolling release distribution and provides lcov in the repositories.

@zappolowski zappolowski merged commit 15b1e8c into dunst-project:master Apr 5, 2024
@zappolowski zappolowski deleted the update-github-action branch April 5, 2024 08:31
@bynect
Copy link
Member

bynect commented Apr 7, 2024

i don't know why but coverage is failing (yesterday was fine though)

@zappolowski
Copy link
Member Author

Yes, I think it's related to this issue.

I'll just update to v4 ... which should work as we're using fedora based images for the coverage reports (IIRC v4 had issues on older ubuntu based images).

@bynect
Copy link
Member

bynect commented Apr 8, 2024

Yes, I think it's related to this issue.

I'll just update to v4 ... which should work as we're using fedora based images for the coverage reports (IIRC v4 had issues on older ubuntu based images).

Thanks

Also, I wanted to make a minor version soon since a lot of commits piled up. I wrote an email to the other maintains but unfortunately I don't have your email. Do you have some prs still unfinished that you want to include?

@zappolowski
Copy link
Member Author

You should be able to get my address from the commits (just look at the signature).

Other than that:

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.

4 participants