Skip to content

Conversation

shaun-nx
Copy link
Contributor

@shaun-nx shaun-nx commented Oct 7, 2025

Proposed changes

This change adds the BuildOS field to our product telemetry.

Closes #4037

Blocked by #4008
Should not be merged until feat/inference-extension is merged

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Release notes

If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.

Collected BuildOS for product telemetry

@shaun-nx shaun-nx requested a review from a team as a code owner October 7, 2025 13:47
@github-actions github-actions bot added enhancement New feature or request tests Pull requests that update tests labels Oct 7, 2025
@shaun-nx shaun-nx added the blocked Blocked by other issue label Oct 7, 2025
Copy link

codecov bot commented Oct 7, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.19%. Comparing base (cb31ea4) to head (06c853d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4038      +/-   ##
==========================================
+ Coverage   86.83%   95.19%   +8.35%     
==========================================
  Files         128        1     -127     
  Lines       16607      229   -16378     
  Branches       62       62              
==========================================
- Hits        14421      218   -14203     
+ Misses       2004       11    -1993     
+ Partials      182        0     -182     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@bjee19 bjee19 left a comment

Choose a reason for hiding this comment

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

nicely done, just a couple of small comments

Comment on lines +182 to +184
if buildOS == "" {
buildOS = "alpine"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this since we have the check in commands.go. So like how we don't add an additional check for ImageSource I think we can follow suite.

Copy link
Contributor

Choose a reason for hiding this comment

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

We'll need to add the field to the "Normal Case"


var _ = Describe("Collector", Ordered, func() {

Describe("BuildOS field", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really think a new Describe block is needed once you add the field in the normal case. Similar to how we don't have a separate Describe block for ImageSource, but if you insist on keeping it, we should move to down towards the other Describe blocks (probably end of file). Currently this is in a weird place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked by other issue enhancement New feature or request release-notes tests Pull requests that update tests
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

OpenShift Support: Product Telemetry
2 participants