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

[githubgen] PR feedback follow-ups #655

Merged

Conversation

mowies
Copy link
Member

@mowies mowies commented Jan 7, 2025

This PR

  • adds unit tests here and there to get coverage to a slightly better level
  • refactors many parts of the code to make it better testable
  • refactors parts of the code to be a bit less performant but much more readable (IMO of course)
  • adds new flags to make the tool more suitable for general usage instead of just being focused on OTel Collector Contrib
    • caveat: the flags for now still have defaults that set everything for OTel Collector Contrib (this should probably change in another follow up to keep this PR from exploding)
  • adds githubgen back to the released packages in versions.yaml
  • addresses all feedback comments from feat: add githubgen tool #639
  • fixes [githubgen] adopt change from contrib #667

Possible follow ups:

  • move the whole CLI flag management to cobra so that flags can be sub-divided per sub-command (codeowners, issue-templates, distributions)
  • if a default codeowner is provided, the tool should not exit with an error when a component doesn't have codeowners define, but it should rather continue with the default codeowners resolved this here because it's a requirement IMHO

Copy link

codecov bot commented Jan 7, 2025

Codecov Report

Attention: Patch coverage is 51.79856% with 67 lines in your changes missing coverage. Please review.

Project coverage is 51.19%. Comparing base (b29324c) to head (7e2fc41).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
githubgen/codeowners.go 64.86% 25 Missing and 1 partial ⚠️
githubgen/main.go 31.57% 26 Missing ⚠️
githubgen/issuetemplates.go 57.89% 8 Missing ⚠️
githubgen/distributions.go 12.50% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #655      +/-   ##
==========================================
+ Coverage   48.83%   51.19%   +2.36%     
==========================================
  Files          57       57              
  Lines        3387     3422      +35     
==========================================
+ Hits         1654     1752      +98     
+ Misses       1572     1507      -65     
- Partials      161      163       +2     

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

@MrAlias MrAlias mentioned this pull request Jan 7, 2025
@mowies mowies force-pushed the githubgen-follow-up-1 branch 3 times, most recently from d10937e to 0196800 Compare January 9, 2025 10:01
@mowies mowies marked this pull request as ready for review January 9, 2025 14:29
@mowies mowies requested review from a team as code owners January 9, 2025 14:29
@mowies mowies requested a review from codeboten January 9, 2025 14:29
@mowies mowies mentioned this pull request Jan 9, 2025
Copy link
Member

@mx-psi mx-psi left a comment

Choose a reason for hiding this comment

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

Still need to look into some files, but some small comments

githubgen/constants.go Outdated Show resolved Hide resolved
githubgen/constants.go Outdated Show resolved Hide resolved
githubgen/constants.go Outdated Show resolved Hide resolved
@mowies
Copy link
Member Author

mowies commented Jan 13, 2025

@dmathieu I addressed all feedback in this PR

githubgen/README.md Outdated Show resolved Hide resolved
@mowies
Copy link
Member Author

mowies commented Jan 14, 2025

i found some new issues with this PR i want to solve before merging, pls don't merge yet

@mx-psi mx-psi marked this pull request as draft January 14, 2025 12:01
@mx-psi
Copy link
Member

mx-psi commented Jan 14, 2025

Let me mark as draft until this is ready to merge

Signed-off-by: Moritz Wiesinger <[email protected]>
Signed-off-by: Moritz Wiesinger <[email protected]>
Signed-off-by: Moritz Wiesinger <[email protected]>
Signed-off-by: Moritz Wiesinger <[email protected]>
Signed-off-by: Moritz Wiesinger <[email protected]>
Signed-off-by: Moritz Wiesinger <[email protected]>
Signed-off-by: Moritz Wiesinger <[email protected]>
@mowies mowies force-pushed the githubgen-follow-up-1 branch from 0a156bc to 497a059 Compare January 14, 2025 14:12
Signed-off-by: Moritz Wiesinger <[email protected]>
Signed-off-by: Moritz Wiesinger <[email protected]>
Signed-off-by: Moritz Wiesinger <[email protected]>
Signed-off-by: Moritz Wiesinger <[email protected]>
Signed-off-by: Moritz Wiesinger <[email protected]>
Signed-off-by: Moritz Wiesinger <[email protected]>
@mowies
Copy link
Member Author

mowies commented Jan 14, 2025

ok the PR should be ready now. i ran into some issues when testing this against the collector core repo with relative paths in codeowner files that were generated. Also, the default codeowners are used when none are defined in a component instead of fully erroring out.

@mowies mowies marked this pull request as ready for review January 14, 2025 16:13
@mowies mowies requested review from atoulme and mx-psi January 14, 2025 16:13
@mowies
Copy link
Member Author

mowies commented Jan 15, 2025

new commits since previous reviews start at b518bbe

@mowies
Copy link
Member Author

mowies commented Jan 16, 2025

@atoulme @mx-psi @dmathieu pls re-review when you have time :)

Signed-off-by: Moritz Wiesinger <[email protected]>
Signed-off-by: Moritz Wiesinger <[email protected]>
# Conflicts:
#	githubgen/codeowners.go
Signed-off-by: Moritz Wiesinger <[email protected]>
@mx-psi
Copy link
Member

mx-psi commented Jan 20, 2025

@atoulme will merge this on Thursday unless you comment

# Conflicts:
#	githubgen/codeowners.go
Copy link
Contributor

@atoulme atoulme left a comment

Choose a reason for hiding this comment

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

sorry didn't realize this was waiting on me, go for it

@mx-psi mx-psi merged commit e1e1314 into open-telemetry:main Jan 22, 2025
12 checks passed
@mowies mowies deleted the githubgen-follow-up-1 branch January 22, 2025 09:57
@jade-guiton-dd
Copy link
Contributor

Would it be okay to include githubgen in the next release now that this is merged?

@mowies
Copy link
Member Author

mowies commented Jan 22, 2025

yes should be ok. i opened a draft release PR already #668

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.

[githubgen] adopt change from contrib
5 participants