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.

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

docs(telemetry): update instructions for disabling telemetry #1715

Conversation

LongLiveCHIEF
Copy link
Contributor

The router requires that this option be a boolean

Fixes #1714

@LongLiveCHIEF
Copy link
Contributor Author

Gotta love CI bugs. @EverlastingBugstopper can you smack the CI runner and see if it recognizes volta on a second attempt?

@EverlastingBugstopper
Copy link
Contributor

yeah volta installs have been unfortunately fairly flaky on windows :( - kicked the build hopefully it works

Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper left a comment

Choose a reason for hiding this comment

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

Thanks for this change, I think it's an improvement since it makes the behavior match the router.

I want to make sure we keep this contract along with the old one working. Would you mind duplicating the it_can_be_disabled test in src/utils/telemetry.rs on line 187 and test the value 1 in addition to the value true?

@LongLiveCHIEF
Copy link
Contributor Author

Sure. I'll be able to get to it later this afternoon.

@LongLiveCHIEF LongLiveCHIEF force-pushed the update-telemetry-env-var-setting branch from 8acbecc to 9db9187 Compare August 21, 2023 12:39
@LongLiveCHIEF LongLiveCHIEF changed the title TEdocs(telemetry): update instructions for disabling telemetry docs(telemetry): update instructions for disabling telemetry Aug 21, 2023
@EverlastingBugstopper EverlastingBugstopper enabled auto-merge (squash) August 21, 2023 13:51
@EverlastingBugstopper
Copy link
Contributor

EverlastingBugstopper commented Aug 21, 2023

I turned on auto-merge for this so it should merge as soon as you update this branch w/latest commits from main and CI passes. I've also merged a PR to main that should fix the flaky Windows error that you encountered on this PR.

@EverlastingBugstopper EverlastingBugstopper self-assigned this Aug 21, 2023
@EverlastingBugstopper EverlastingBugstopper added the docs 📝 improvements or additions to docs on the docs site label Aug 21, 2023
@EverlastingBugstopper EverlastingBugstopper added this to the v0.18.1 milestone Aug 21, 2023
auto-merge was automatically disabled August 22, 2023 12:11

Head branch was pushed to by a user without write access

@LongLiveCHIEF LongLiveCHIEF force-pushed the update-telemetry-env-var-setting branch from 9db9187 to 9a3a864 Compare August 22, 2023 12:11
@LongLiveCHIEF
Copy link
Contributor Author

I turned on auto-merge for this so it should merge as soon as you update this branch w/latest commits from main and CI passes. I've also merged a PR to main that should fix the flaky Windows error that you encountered on this PR.

It's passed all checks, and rebased against main, but still not merging for some reason.

@EverlastingBugstopper EverlastingBugstopper merged commit 27e2531 into apollographql:main Aug 22, 2023
@EverlastingBugstopper
Copy link
Contributor

Huh, it must have turned off auto-merge after rebasing from main. Regardless - it's merged now, thanks for the contribution!

@LongLiveCHIEF
Copy link
Contributor Author

Huh, it must have turned off auto-merge after rebasing from main. Regardless - it's merged now, thanks for the contribution!

lol, it's a self-defeating prophecy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs 📝 improvements or additions to docs on the docs site
Projects
None yet
Development

Successfully merging this pull request may close these issues.

curl installation suggests APOLLO_TELEMETRY_DISABLED=1 which seems wrong
2 participants