-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[gh-action]: Generate a weekly report of activity #26111
[gh-action]: Generate a weekly report of activity #26111
Conversation
|
didn't add a changelog entry since this was an internal facing change. happy to add an entry if we feel this warrants that |
updated issue format with gh style checkboxes: kevinslin#21 |
looks like the gh workflows requires an approval from a maintainer? |
// Check if directory exists | ||
let files; | ||
try { | ||
files = fs.readdirSync(startPath); |
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.
Why not use the async versions of the fs functions?
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.
the logic was easier to follow when sticking with sync. since the number of directories isn't that large, felt like this wouldn't be a bottleneck
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.
the logic was easier to follow when sticking with sync.
Not sure I understand. The flow of the logic is the same with async, you just add the await
keyword.
files = await fs.readdir(startPath);
The await
keyword is already used elsewhere in the script, e.g.
const response = await octokit.issues.listForRepo(queryParams);
I thought maybe it's an issue in importing the fs/promises
version of the fs
library?
const results = {}; | ||
|
||
for (let filePath of files) { | ||
const component = path.basename(path.dirname(path.dirname(filePath))); // Top level directory |
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.
If I understand correctly, by component
you mean component class
- a receiver, or a processor, etc. Reading the component type from the path should work, but a cleaner way would be to use the status.class
field from the metadata file contents.
You could also read the component's name from type
field in the metadata file.
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 is because we currently collect metadata.yaml
from everything. this includes internal components that don't have a status.class
field (eg. processor/resourcedetectionprocessor/internal/aws/ec2/metadata.yaml). though to be fair. added a check for the status.class
to set component and default to the current method if nothing is found
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.
All my comments are minor things that can be either ignored or improved upon in separate PRs. This is good to be merged in my view 👍
Co-authored-by: Antoine Toulme <antoine@toulme.name>
Co-authored-by: Antoine Toulme <antoine@toulme.name>
also switch out json links with html links
0f693a0
to
7a11c9e
Compare
finished addressing pr changes. thanks for the review @astencel-sumo |
checking if there are any other blockers for this pr? @atoulme do you have authorization to merge this pr? |
I am a little concerned that this introduces javascript to our repository. @open-telemetry/collector-contrib-approvers is everyone willing to take on this language dependency? |
its a valid concern. i just want to point out that there is already javascript in the repository (embedded inside of yaml). |
Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
I've worked with some decently large Node.js apps in my past, I'd be willing to take it. Unless we'd rather have this report generator written in a different language, that more contributors of this repo are comfortable with. What would it be? Perhaps Python? |
I'm fine helping with js code. |
Gonna give one more day for approvers/maintainers to speak up, then I'll merge. |
Speaking as someone who has written a handful of GitHub workflows as Bash scripts that call the GitHub CLI, I think writing workflows in a programming language that use an API client library is a better alternative: it's much easier to reason about and should be more straightforward to test locally. I have pretty extensive JavaScript experience, so I would be happy to help maintain JavaScript workflows. Looking at our options, between the three language implementations that GitHub provides SDKs for, I think JavaScript is the most likely to be the best understood both within the Collector SIG and across the wider OTel community (I'd like to maintain an eye toward possibly publishing our actions to be used in other repos). I think using Go would also be an obvious choice, and there is a third-party API client for GitHub written in Go, but I can't comment on its quality compared to the official JavaScript SDK. |
**Description:** This PR creates a gh action that generates a weekly report on repo statistics. It delivers on the requirements specified in open-telemetry#24672 You can see the sample output here: kevinslin#16 **Link to tracking Issue:** open-telemetry#24672 **Testing:** Manual testing in fork: https://github.com/kevinslin/opentelemetry-collector-contrib/actions Example output: kevinslin#17 **Documentation:** The architecture: - we use `actions/github-script@v6` to make calls to the gh apis - we require installing `js-yaml` in order to parse `metadata.yaml` files in order to get components. this dependency is installed during the github action run and not persisted Some caveats about the logic: - when this action runs, it looks back the previous 7 days and gets issues created in that time period, normalizing times to UTC - eg. if running this on wednesday (eg. 2023-08-25 17:35:00), it will scan issues from the previous wednesday (2023-08-18 00:00:00Z) to beginning of this wednesday (2023-08-28 0:00:00Z) - this action writes the json payload of the report inside the issue. the payload is parsed by future reports to calculate deltas - the report issue has a custom label: `report` - this is used so we can properly filter previous issues when calculating deltas. the [github issues api](https://docs.github.com/en/rest/issues/issues?apiVersion=2022-11-28#list-repository-issues) only does filtering based on labels and `since` date This action currently runs every Tuesday at 1AM UTC --------- Co-authored-by: Antoine Toulme <antoine@toulme.name> Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
Description:
This PR creates a gh action that generates a weekly report on repo statistics.
It delivers on the requirements specified in #24672
You can see the sample output here: kevinslin#16
Link to tracking Issue: #24672
Testing:
Manual testing in fork: https://github.com/kevinslin/opentelemetry-collector-contrib/actions
Example output: kevinslin#17
Documentation:
The architecture:
actions/github-script@v6
to make calls to the gh apisjs-yaml
in order to parsemetadata.yaml
files in order to get components. this dependency is installed during the github action run and not persistedSome caveats about the logic:
report
- this is used so we can properly filter previous issues when calculating deltas. the github issues api only does filtering based on labels andsince
dateThis action currently runs every Tuesday at 1AM UTC