-
Notifications
You must be signed in to change notification settings - Fork 458
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
Checkpoint telemetry integration #326
Conversation
5c2ed94
to
c29762b
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.
Besides from the comments, are we going to document this somehow?
packages/cdktf-cli/lib/checkpoint.ts
Outdated
|
||
export async function ReportRequest(reportParams: ReportParams): Promise<void> { | ||
// we won't report when checkpoint is disabled. | ||
if (process.env.CDKTF_CHECKPOINT_DISABLE == "1") { |
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.
should this be set for our tests - so pretty much for all CI jobs?
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.
Yes, I think that would help give with generating accurate telemetry.
I have documented the telemetry here. |
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.
Looks good.
What do you think about adding one explicit (integration) test for this telemetry stuff?
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.
As just discussed, just merge after adding the integration test
I am seeing errors that aren't related to the PR. I think merging #276 has started causing these errors. |
I'm looking into it. |
6b431dc
to
72e05ad
Compare
I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Integrating Checkpoint telemetry to report project version, provider name, provider version, etc.
No personal data needs to be collected through this telemetry option. Like all HashiCorp projects using Checkpoint, we have an environmental variable
CHECKPOINT_DISABLE
that can be set to a non empty value for opting out of this reporting.Fixes: #325