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

Default to not sending data to Sentry for now #1695

Merged
merged 3 commits into from
Sep 14, 2022

Conversation

chadwhitacre
Copy link
Member

@chadwhitacre chadwhitacre commented Sep 14, 2022

Follow-up to #1679 to relax the non-interactive experience, closes #1693.

The proposal here is to default to not sending data for the non-interactive case, but with a notice that we will be forcing a decision in an upcoming version. This gives folks three months to notice and adjust, and it gives us some time to clean up configuration a bit.

@chadwhitacre
Copy link
Member Author

What do you think of this approach @Dherlou?

@Dherlou
Copy link

Dherlou commented Sep 14, 2022

@chadwhitacre
My personal opinion is that data privacy matters should always be opt-in and not opt-out. I am from Germany, so I am used to work according to the EU GDPR, which seems to be in contrast to e.g. CCPA where it is the other way around and opt-out is typically the default.
That said, if you want to force users to make an explicit choice to a later point of time anyway, I would recommend just showing the message in non-interactive-mode and exit with status code 1. People can then respond to the resulting error in CI/CD pipelines by explicitly making a decision and implementing it in their CI/CD pipeline (e.g. by echoing 'yes' or 'no' in the '.reporterrors' as a build step or use the environment variable once implemented) instead of (probably) unknowingly sending their data by default without an explicit consent. I doubt that people will read the output of their pipelines as long as it 'still works'. :D

@chadwhitacre
Copy link
Member Author

chadwhitacre commented Sep 14, 2022

I doubt that people will read the output of their pipelines as long as it 'still works'. :D

I know I don't! 😜

showing the message in non-interactive-mode and exit with status code 1

I think for tomorrow it's best to show the message but continue with the installation (defaulting to not sending, of course). When we add the exit in 22.12.0 we will be able to say "We gave you three months to respond." Better than abrupt change.

@chadwhitacre chadwhitacre force-pushed the cwlw/default-opt-out-on-error-reporting-for-now branch from c18b9ce to 3099b61 Compare September 14, 2022 15:47
@ethanhs
Copy link
Contributor

ethanhs commented Sep 14, 2022

I think for tomorrow it's best to show the message but continue with the installation (defaulting to opt-out, of course). When we add the exit in 22.12.0 we will be able to say "We gave you three months to respond." Better than abrupt change.

I don't think there is any point in waiting 3 months, the main way people are going to notice this change is reading the changelog, so I think doing one release with it default opt-out, then making choosing opt-in or out mandatory in the next release is a good idea.

@chadwhitacre
Copy link
Member Author

chadwhitacre commented Sep 14, 2022

I don't think there is any point in waiting 3 months, the main way people are going to notice this change is reading the changelog, so I think doing one release with it default opt-out, then making choosing opt-in or out mandatory in the next release is a good idea.

Fair enough. Changed to 22.10.0 in 109036d, and also stopped mis-using "opt-out." It's not "default to opt-out," it's that opt-in is "default to not send." Next month it'll be neither opt-in nor opt-out, we'll force a choice.

@chadwhitacre chadwhitacre enabled auto-merge (squash) September 14, 2022 17:16
@chadwhitacre chadwhitacre changed the title Default opt-out on error reporting for now Default to not sending data to Sentry for now Sep 14, 2022
@hubertdeng123
Copy link
Member

I'm guessing this PR will be going in before the others? Also agree that 1 month should be enough since it's not a change most people will even notice

@chadwhitacre
Copy link
Member Author

this PR will be going in before the others

Yes, let's get this landed ASAP as it is the most vital to not making everyone mad tomorrow. :)

I've pushed 75c713e so we can see this working in our two CI environments. I was seeing different behavior between the two with an earlier implementation of PROMPTABLE (GitHub provides stdin whereas GCP does not).

Copy link
Contributor

@ethanhs ethanhs left a comment

Choose a reason for hiding this comment

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

Looks like this is working! Just need to revert the test commit and this should be good I think?

@hubertdeng123
Copy link
Member

Should we revert the changes we made yesterday to the other cloudbuild.yaml files once this goes in? I think ideally those should also be changed

@chadwhitacre
Copy link
Member Author

Reporting turned off again in CI in ea809d82e853da57c08c9121100452e70104b6ac. I think we can do something similar in sentry and snuba, sound right?

@chadwhitacre chadwhitacre force-pushed the cwlw/default-opt-out-on-error-reporting-for-now branch from ea809d8 to 75c713e Compare September 14, 2022 18:53
@chadwhitacre
Copy link
Member Author

Oops, nevermind ... forgot we don't have those yet. 🤪

Let's take care of that in #1697.

@hubertdeng123
Copy link
Member

Ah yeah, that's what I meant. Once we change completely from using .reporterrors we can probably go ahead in sentry and snuba to remove references to the .reporterrors file

@chadwhitacre chadwhitacre enabled auto-merge (squash) September 14, 2022 18:57
@chadwhitacre
Copy link
Member Author

Turned on automerge. Better withdraw your ✅ if you don't want this merged, @ethanhs. 😄

@chadwhitacre chadwhitacre merged commit 2ef54aa into master Sep 14, 2022
@chadwhitacre chadwhitacre deleted the cwlw/default-opt-out-on-error-reporting-for-now branch September 14, 2022 19:11
@ethanhs
Copy link
Contributor

ethanhs commented Sep 14, 2022

Turned on automerge. Better withdraw your ✅ if you don't want this merged

Oh, but I do! :)

@github-actions github-actions bot locked and limited conversation to collaborators Sep 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve error handling handling in non-interactive case
4 participants