-
Notifications
You must be signed in to change notification settings - Fork 362
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
ci: disable telemetry for ci tests #9671
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9671 +/- ##
==========================================
- Coverage 53.34% 53.33% -0.01%
==========================================
Files 1255 1255
Lines 152702 152702
Branches 3250 3249 -1
==========================================
- Hits 81455 81451 -4
- Misses 71095 71099 +4
Partials 152 152
Flags with carried forward coverage won't be shown. Click here to find out more. |
@@ -29,6 +29,8 @@ stages: | |||
initial_user_password: $INITIAL_USER_PASSWORD | |||
authz: | |||
rbac_ui_enabled: true | |||
telemetry: |
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.
question I'm surprise, were they actually reporting anything? wouldn't the default be false? did they have the api key needed to report?
SegmentMasterKey string `json:"segment_master_key"` |
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 have no idea. What's the impact either way?
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 it defaults to false, perhaps change the name of the PR to better reflect that
don't see an issue to merging anyway though
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 defaults to true. https://docs.determined.ai/latest/reference/deploy/master-config-reference.html#id29
Infra noticed pretty quick growth in segment data coming from ci: https://hpe-aiatscale.slack.com/archives/C03JGLKTBEJ/p1720548565903099
Good point on the API key; I don't know how it was successfully reported.
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.
LGTM
@@ -14,3 +14,6 @@ db: | |||
# password: database_password | |||
|
|||
launch_error: false | |||
|
|||
telemetry: | |||
enabled: false |
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.
missing blank line at end?
cbf44df
to
d6479c1
Compare
Ticket
RM-374
Description
Reduce telemetry usage for ci tests.
Test Plan
Checklist
docs/release-notes/
See Release Note for details.