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 : added merge group support + action.yml for github action #129

Merged
merged 12 commits into from
Sep 20, 2024

Conversation

EthanPERRUZZA
Copy link
Contributor

Hey there!

In some of our project repositories, we use the merge-group feature, and when trying to use your script, we noticed it wasn't working as expected. To address this, we wrote some additional code to ensure compatibility and would love to contribute it back.

While working on this, we also added an action.yml file for easier integration with GitHub Actions and to simplify the workflow syntax. Let us know if you have any feedback or suggestions!

Thanks!

Signed-off-by: Ethan Perruzza <[email protected]>
Copy link

codecov bot commented Sep 16, 2024

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 57.58%. Comparing base (30353d8) to head (d786340).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
dco_check/dco_check.py 0.00% 6 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (30353d8) and HEAD (d786340). Click for more details.

HEAD has 6 uploads less than BASE
Flag BASE (30353d8) HEAD (d786340)
7 1
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #129      +/-   ##
==========================================
- Coverage   66.27%   57.58%   -8.70%     
==========================================
  Files           2        2              
  Lines         516      521       +5     
  Branches       88       89       +1     
==========================================
- Hits          342      300      -42     
- Misses        150      200      +50     
+ Partials       24       21       -3     
Flag Coverage Δ
57.58% <0.00%> (?)

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.

@EthanPERRUZZA
Copy link
Contributor Author

@christophebedard I don't know if you get notifications for PRs on your repo, sorry if it double pinged you. Could you check that please ?

@christophebedard
Copy link
Owner

Thanks for the PR! Yes I definitely get notifications for PRs! I was on vacation and away from my computer; I'm looking at the PR now.

@christophebedard
Copy link
Owner

Looks like CI is failing with Python 3.7. Not sure if it's also failing with Python >3.7, but you could remove 3.7 from the matrix, since it's now end-of-life (and maybe add 3.12): https://devguide.python.org/versions/#python-release-cycle.

action.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@christophebedard
Copy link
Owner

Other than the (numerous) small suggestions and requests, this looks good!

@christophebedard christophebedard added the enhancement New feature or request label Sep 18, 2024
@EthanPERRUZZA
Copy link
Contributor Author

Hello, I applied your suggestion.
I had to add back the 0.5.0 modification as I need the tag in the workflow to test it. However it seems that the test needs your approval to run and I'm not sure if tags get carried along with merge request.

README.md Outdated Show resolved Hide resolved
@christophebedard
Copy link
Owner

I had to add back the 0.5.0 modification as I need the tag in the workflow to test it.

I don't think the __version__ value is needed for uses: christophebedard/[email protected]. You could just point the workflow to your PR branch to test for now (EthanPERRUZZA/dco-check@master), and we can switch it to christophebedard/dco-check@master before merging.

@christophebedard
Copy link
Owner

I see that all the Test / test jobs are failing due to Codecov. You can ignore that; I'll fix this in another PR.

action.yml Outdated Show resolved Hide resolved
@christophebedard
Copy link
Owner

christophebedard commented Sep 20, 2024

Sorry for the delayed (re)reviews, I've been pretty busy over the last few days. I will unfortunately be away from my computer once again starting in a few hours until Friday 1 week from now. However, I'll probably be able to rereview and merge this once the action works.

@EthanPERRUZZA
Copy link
Contributor Author

Hey, no worries, thank you for your time each day for the review ;)

@christophebedard christophebedard merged commit c9ae1d5 into christophebedard:master Sep 20, 2024
7 of 17 checks passed
@christophebedard
Copy link
Owner

Merci!

@christophebedard
Copy link
Owner

I've created a 0.5.0 tag. Looks like the full release workflow (for PyPI) isn't working: https://github.com/christophebedard/dco-check/actions/runs/10966330308/job/30454028221. I'll have to fix it next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants