-
Notifications
You must be signed in to change notification settings - Fork 200
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
adding telemetry #1692
adding telemetry #1692
Conversation
7d52daa
to
2b4f54b
Compare
953f9b8
to
6e97890
Compare
8a76653
to
7a6f605
Compare
8410ebb
to
8d5473a
Compare
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.
What can we do to make this less verbose?
3401de0
to
c18b8bd
Compare
|
||
telemetry-summarize: | ||
runs-on: ubuntu-latest | ||
needs: pr-builder |
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.
pr-builder has always been the last job until now. PRs can be merged once pr-builder completes. Is it important for this to complete before merges occur?
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.
It was easy to piggy-back on pr-builder being the last thing so that everything else is done. It is not important for this to be complete before merges occur, but it is important that it start only after all other jobs (except pr-builder) are complete. Is there a better way to express that? Making it need conda-python-tests and wheel-python-tests would probably be good enough, and then this would run in parallel with pr-builder. I find that conceptually more messy, but I'm fine with it from a practical standpoint.
If the concern is that the merge would mess up the telemetry, I think that's also going to be OK. The telemetry is all based on github actions run metadata, and I believe that continues to exist until it ages out after some amount of time. The few seconds between a merge event and the summarize action won't matter.
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.
Great. I am fine with this depending on pr-builder.
d78c7eb
to
03299cb
Compare
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.
One suggestion for moving code, otherwise LGTM.
/merge |
Description
close #1691
Checklist