Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow controlling telemetry via the ODO_TRACKING_CONSENT environment variable #6258

Merged
merged 11 commits into from
Oct 31, 2022
Merged

Allow controlling telemetry via the ODO_TRACKING_CONSENT environment variable #6258

merged 11 commits into from
Oct 31, 2022

Conversation

rm3l
Copy link
Member

@rm3l rm3l commented Oct 26, 2022 •
edited
Loading

What type of PR is this:
/kind feature
/area telemetry

What does this PR do / why we need it:
See #6253

Which issue(s) this PR fixes:
Fixes #6253

PR acceptance criteria:

How to test changes / Special notes to the reviewer:

@rm3l rm3l requested a review from kadel October 26, 2022 14:32
@openshift-ci openshift-ci bot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Oct 26, 2022
@netlify
Copy link

netlify bot commented Oct 26, 2022 •
edited
Loading

✅ Deploy Preview for odo-docusaurus-preview ready!

Name Link
🔨 Latest commit 8b69040
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/635fce21e66a7900077ca9d4
😎 Deploy Preview https://deploy-preview-6258--odo-docusaurus-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@openshift-ci openshift-ci bot added the area/telemetry Issues or PRs related to telemetry or metrics collection label Oct 26, 2022
@odo-robot
Copy link

odo-robot bot commented Oct 26, 2022 •
edited
Loading

Unit Tests on commit 24c29e7 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Oct 26, 2022 •
edited
Loading

Validate Tests on commit 24c29e7 finished successfully.
View logs: TXT HTML

@rm3l rm3l requested review from valaparthvi and removed request for rnapoles-rh October 26, 2022 14:49
@odo-robot
Copy link

odo-robot bot commented Oct 26, 2022 •
edited
Loading

Kubernetes Tests on commit 24c29e7 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Oct 26, 2022 •
edited
Loading

Windows Tests (OCP) on commit 24c29e7 finished successfully.
View logs: TXT HTML

@rm3l rm3l mentioned this pull request Oct 26, 2022
Merged
3 tasks
@odo-robot
Copy link

odo-robot bot commented Oct 26, 2022 •
edited
Loading

OpenShift Tests on commit 24c29e7 finished successfully.
View logs: TXT HTML

@kadel
Copy link
Member

kadel commented Oct 27, 2022

One thing that I have noticed is that if I do odo preference set ConsentTelemetry false and then ODO_TRACKING_CONSENT=yes odo list -v 4 it won't track the odo list command.

We haven't discussed this, but I think that the env variable should override whatever is set in preference.

@rm3l
Copy link
Member Author

rm3l commented Oct 27, 2022

One thing that I have noticed is that if I do odo preference set ConsentTelemetry false and then ODO_TRACKING_CONSENT=yes odo list -v 4 it won't track the odo list command.

We haven't discussed this, but I think that the env variable should override whatever is set in preference.

Indeed, that makes sense. I'll change that behavior.

rm3l referenced this pull request in rm3l/odo Oct 27, 2022
@rm3l
Make ODO_TRACKING_CONSENT env var take precedence over the ConsentTel…
cfc6464 
…emetry preference

See [1] for more context.

[1] https://github.com/redhat-developer/odo/pull/6258\#issuecomment-1293736398
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Oct 27, 2022
@rm3l
Copy link
Member Author

rm3l commented Oct 27, 2022 •
edited
Loading

One thing that I have noticed is that if I do odo preference set ConsentTelemetry false and then ODO_TRACKING_CONSENT=yes odo list -v 4 it won't track the odo list command.
We haven't discussed this, but I think that the env variable should override whatever is set in preference.

Indeed, that makes sense. I'll change that behavior.

Done in f8e109591.
I'll rebase the branch later on to fix the conflicts.

@rm3l rm3l removed the request for review from dharmit October 27, 2022 16:21
rm3l referenced this pull request in rm3l/odo Oct 27, 2022
@rm3l
Make ODO_TRACKING_CONSENT env var take precedence over the ConsentTel…
f8e1095 
…emetry preference

See [1] for more context.

[1] https://github.com/redhat-developer/odo/pull/6258\#issuecomment-1293736398
@rm3l rm3l force-pushed the 6253-env-variable-to-enable-telementry branch from cfc6464 to f8e1095 Compare October 27, 2022 21:21
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Oct 27, 2022
@rm3l
Copy link
Member Author

rm3l commented Oct 28, 2022 •
edited
Loading

Rebased and force-pushed to fix conflicts with documentation.

...
Ran 396 of 396 Specs in 768.518 seconds
SUCCESS! -- 396 Passed | 0 Failed | 0 Pending | 0 Skipped


Ginkgo ran 1 suite in 13m49.56691004s
Test Suite Passed
go run -mod=vendor github.com/onsi/ginkgo/v2/ginkgo  --randomize-all --slow-spec-threshold=120s -timeout 14400s --no-color -nodes=32 tests/e2escenarios
Running Suite: odo e2e scenarios - /go/odo_1/tests/e2escenarios
===============================================================
Random Seed: 1666906779 - will randomize all specs

...
Timed out after 120.000s.
  Expected
      <string>: Hello from Node.js Starter Application!
  to equal
      <string>: Hello from updated Node.js Starter Application!
  In [It] at: /go/odo_1/tests/e2escenarios/e2e_test.go:37
------------------------------


Summarizing 2 Failures:
  [FAIL] E2E Test starting with empty Directory [It] should verify developer workflow from empty Directory
  /go/odo_1/tests/e2escenarios/e2e_test.go:37
  [FAIL] E2E Test starting with non-empty Directory [It] should verify developer workflow from non-empty Directory
  /go/odo_1/tests/e2escenarios/e2e_test.go:37

Ran 6 of 6 Specs in 177.275 seconds
FAIL! -- 4 Passed | 2 Failed | 0 Pending | 0 Skipped

E2E Tests fixed by #6260

feloy
feloy reviewed Oct 28, 2022
View reviewed changes
pkg/segment/segment.go Outdated
} else if cfg.GetConsentTelemetry() {
}

if trackingConsent == "yes" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have functions trackingConsentEnabled/trackingConsentDisabled instead, so we could rename the values or add aliases later?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done - added in 48669d3 (#6258).

This made me realize that I had few cases not tested, e.g., when the variables are not present in the env. So I added more unit tests in 75f2321 (#6258)

kadel and others added 5 commits October 31, 2022 10:05
@kadel @rm3l
Add support for ODO_TRACKING_CONSENT env variable
54d5273 
Co-authored-by: Armel Soro <asoro@redhat.com>
@rm3l
Replace deprecated 'ODO_DISABLE_TELEMETRY' env var with 'ODO_TRACKING…
de7c17c 
…_CONSENT' in integration tests
@rm3l
Log environment of process launched with 'helper_run' in integration …
5758651 
…tests

`odo` behavior might be altered based on certain environment variables.
So this is to help debug future issues that might happen.

Because the process environment also contains the
current OS environment, we purposely limit the content
to variables prefixed with 'ODO_' or particular ones
(like 'TELEMETRY_CALLER').
@rm3l
Test conflicting situations when using both 'ODO_DISABLE_TELEMETRY' a…
f227bd2 
…nd 'ODO_TRACKING_CONSENT'
@rm3l
Disable golangci-lint 'staticcheck' check about using the deprecated …
80740f6 
…'segment.DisableTelemetryEnv'

However, due to [1], line-based directives do not seem to be working.
@rm3l rm3l force-pushed the 6253-env-variable-to-enable-telementry branch from 551198e to f014337 Compare October 31, 2022 09:05
@rm3l
Copy link
Member Author

rm3l commented Oct 31, 2022

Had to rebase and force-push to fix the integration tests (#6260).

@feloy feloy closed this Oct 31, 2022
@feloy feloy reopened this Oct 31, 2022
@sonarcloud
Copy link

sonarcloud bot commented Oct 31, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@feloy
Copy link
Contributor

feloy commented Oct 31, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Oct 31, 2022
@feloy
Copy link
Contributor

feloy commented Oct 31, 2022

/approve
/override ci/prow/v4.11-integration-e2e
All tests pass on IBM Cloud

@rm3l
Copy link
Member Author

rm3l commented Oct 31, 2022

@kadel Can you please take another look at this PR and approve if it is okay for you? Thanks.

@openshift-ci
Copy link

openshift-ci bot commented Oct 31, 2022

@feloy: Overrode contexts on behalf of feloy: ci/prow/v4.11-integration-e2e

In response to this:

/approve
/override ci/prow/v4.11-integration-e2e
All tests pass on IBM Cloud

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci
Copy link

openshift-ci bot commented Oct 31, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feloy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Oct 31, 2022
@openshift-merge-robot openshift-merge-robot merged commit 719fe99 into redhat-developer:main Oct 31, 2022
@rm3l rm3l deleted the 6253-env-variable-to-enable-telementry branch October 31, 2022 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feloy feloy feloy left review comments

@kadel kadel Awaiting requested review from kadel

@valaparthvi valaparthvi Awaiting requested review from valaparthvi

Assignees

@feloy feloy

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. area/telemetry Issues or PRs related to telemetry or metrics collection kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

env variable to enable telemetry
4 participants
@rm3l @kadel @feloy @openshift-merge-robot