-
Notifications
You must be signed in to change notification settings - Fork 44
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
Refactor(issuegenerator): Create issues based on JUnit test results #672
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Arthur Silva Sens <[email protected]>
I'm opening it as a draft first so people can chime in on the approach :) This is still not working 100% because I'm failing to correlate module names with the appropriate filepaths that are present in the codeowners file. A module name has the full name that is present in the go.mod file, while the pattern used in the codeowners files uses only the relative path of files. 😕 |
Signed-off-by: Arthur Silva Sens <[email protected]>
Signed-off-by: Arthur Silva Sens <[email protected]>
Signed-off-by: Arthur Silva Sens <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #672 +/- ##
==========================================
- Coverage 52.49% 52.43% -0.07%
==========================================
Files 57 57
Lines 3444 3477 +33
==========================================
+ Hits 1808 1823 +15
- Misses 1466 1480 +14
- Partials 170 174 +4 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Arthur Silva Sens <[email protected]>
Signed-off-by: Arthur Silva Sens <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reasonable to me, have you done any tests with real contrib data? I would be willing to try directly on contrib, if we can restrict it to a just a few modules to begin with (e.g. we could begin with just extensions)
I haven't tried with contrib, but with my fork of collector-core. I've just added one commit updating the codeowners (This test was done before removing the codeowners bit from this PR). The issue was opened without problems: ArthurSens/opentelemetry-collector#1 . This comment was after removing the codeowners bit.
I can see two ways forward here:
|
I think we can do that using some conditionals on Github Actions, no need to modify this tool |
Signed-off-by: Arthur Silva Sens <[email protected]>
Signed-off-by: Arthur Silva Sens <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- please either use methods that always have the value receiver or the object receiver :) right now, it's a mix of both
- i like it more when types/consts/vars etc. are at the top of the file and then the funcs follow that
rg.logger.Info("No existing Issues found, creating a new one.") | ||
createdIssue := rg.createIssue(report) | ||
rg.logger.Info("New GitHub Issue created", zap.String("html_url", *createdIssue.HTMLURL)) | ||
rg.reportIterator++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be moved out of the if block
@@ -115,9 +179,9 @@ type reportGenerator struct { | |||
func (rg *reportGenerator) getRequiredEnv() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think the doc string for this method needs an update
One of our goals for the collector is to be more proactive when detecting flaky tests on
main,
as mentioned in open-telemetry/opentelemetry-collector-contrib#36761. Since tests must pass before getting merged tomain
, it's safe to assume that the test is flaky if it fails on themain
branch.Thanks to open-telemetry/opentelemetry-collector#11963, we can use JUnit test results to populate this tool.