-
Notifications
You must be signed in to change notification settings - Fork 330
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
Add runner OS and arch to status report #959
Conversation
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'm pretty sure status reports will fail to upload because of the new fields until we can make the server side changes.
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.
Nice. Just be sure to get the hydro schema changes deployed before merging this.
Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>
a6bd509
to
870dbaa
Compare
@@ -692,6 +695,9 @@ export async function createStatusReportBase( | |||
if (matrix) { | |||
statusReport.matrix_vars = matrix; | |||
} | |||
if (runnerOs === "Windows" || runnerOs === "macOS") { | |||
statusReport.runner_os_release = os.release(); |
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.
Is there a reason not to do this on linux? I just tried on one of my codespaces and I get:
Welcome to Node.js v14.17.6.
Type ".help" for more information.
> require('os').release()
'5.4.0-1069-azure'
It's not in x.y.z form, but it's still valid. I think it's the kernel version.
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.
There are a few reasons.
- We want to limit the cardinality of the status values reported, and there are many more variations for Linux kernel release strings than for Windows and macOS release versions.
- Linux kernel release IDs may contain custom strings, and we want to avoid recording private data (for custom kernel builds on self-hosted runners, for example).
- Windows and macOS release strings are more useful because they are representative of the entire system configuration (whether you are running Windows 10 or Windows 11, for example). Linux kernel versions are less coupled with the userspace environment—5.11.3 on RedHat could be very different from 5.11.3 on Ubuntu.
So we are recording OS releases only for Windows and macOS.
Merge / deployment checklist
This PR adds the OS and architecture (as reported by the GitHub Action Runner environments
RUNNER_OS
andRUNNER_ARCH
) to the status report.