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

Provide GNU test comparison comments for PRs in Github Actions. #400

Merged
merged 27 commits into from
Jun 23, 2024

Conversation

hanbings
Copy link
Collaborator

@hanbings hanbings commented Jun 7, 2024

link: #360

Add the PR comment code to compat.yml.
But I don't have a good way to test this code yet, so I'm setting it to Daft for now.

Copy link

codecov bot commented Jun 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.48%. Comparing base (84e4be8) to head (1cd319d).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #400   +/-   ##
=======================================
  Coverage   60.48%   60.48%           
=======================================
  Files          32       32           
  Lines        4069     4069           
  Branches      895      895           
=======================================
  Hits         2461     2461           
  Misses       1254     1254           
  Partials      354      354           

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

@sylvestre
Copy link
Contributor

nice !
could you please fix the conflict? thanks

@sylvestre
Copy link
Contributor

Testing is hard, don't hesitate if you need to land it to test it further

@hanbings
Copy link
Collaborator Author

Testing is hard, don't hesitate if you need to land it to test it further

It seems that the compatibility information in Annotations is extracted correctly, but Actions gets a 403 unauthorized error when posting a comment. I guess the Token is not valid. Do we have a Secret named GITHUB_TOKEN or another Secret with comment permissions set in the repo?

and... I'm sorry for spending so much time testing.

@sylvestre
Copy link
Contributor

@tertsdiepraam do you remember if it can be tested inside a PR or it needs to be merged first ?

@tertsdiepraam
Copy link
Member

I think it had to be merged...

@tertsdiepraam
Copy link
Member

Also, I split it into 2 workflows on purpose, so that the workflow that posts the comments could have more permissions but the other workflow could be run from the PR. We should probably do the same here. See also some of the discussions and stuff around this feature in coreutils: uutils/coreutils#4010

@hanbings
Copy link
Collaborator Author

I'm separating the code for sending PR comments into a new CI file, but Github Actions only runs the CI file that exists in the default branch. I'm not sure the code will work, can we merge this PR first and then fix the code issues?

@sylvestre
Copy link
Contributor

sure, i guess it isn't a draft anymore then ?

@hanbings hanbings marked this pull request as ready for review June 23, 2024 16:41
@sylvestre sylvestre merged commit d329702 into uutils:main Jun 23, 2024
18 checks passed
@sylvestre
Copy link
Contributor

done

@sylvestre
Copy link
Contributor

@hanbings
Copy link
Collaborator Author

hanbings commented Jun 23, 2024

yeah, I feel weird about that. I'll fix it in my fork. Thanks.

@sylvestre
Copy link
Contributor

yeah, I feel weird about that. I'll fix it in my fork. Thanks.

don't hesitate to copy what is done in the coreutils :)

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.

Just like with rust/coreutils, compare with the GNU implementation and highlight improvements and regressions
3 participants