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

Disable Watson TTS Telemetry #26253

Merged
merged 5 commits into from Sep 12, 2019

Conversation

@poofyteddy
Copy link
Contributor

commented Aug 28, 2019

Description:

Will disable telemetry by default for everyone. But i don't see this as a bad thing, maybe i'm wrong

As in the doc https://cloud.ibm.com/apidocs/text-to-speech?code=python#data-collection
I think leaving telemetry enable break the idea of privacy a self hosted solution like HA allow. So this option felt necessary.

Can't test it right now because HA is down for unrelated reason
First time i do any edition to HA, so i surely missed stuff, plus i am really not a dev ...

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here> Not needed, no change to the front end

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
poofyteddy added 2 commits Aug 28, 2019
As in the doc https://cloud.ibm.com/apidocs/text-to-speech?code=python#data-collection
Still need to understand how config work in ha
This should do it from what i understand
@homeassistant

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

Hi @poofyteddy,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

Copy link
Contributor

left a comment

Do we even need the option, why would anyone ever enable it?

homeassistant/components/watson_tts/tts.py Outdated Show resolved Hide resolved
Dev automation moved this from Needs review to Review in progress Aug 28, 2019
@homeassistant

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

Hi @poofyteddy,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@homeassistant homeassistant added cla-signed and removed cla-needed labels Aug 28, 2019
@project-bot project-bot bot moved this from Review in progress to Needs review in Dev Aug 28, 2019
@amelchio

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

It was actually better before. We prefer positively named options because double negatives like disable_telemetry: false get weird.

But no option is even better, so please answer my above question: Do we even need the option, why would anyone ever enable it?

@poofyteddy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 28, 2019

Sorry @amelchio, Github is an unfriendly thing for me, and i was looking for a reply button ...
In the way i see thing, you should never remove an option. Giving the user a chose about it feel right.
99% of people won't actively look to enable it, but i think there is a 1% who wish to help development of the tech by sharing data

@amelchio

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

In the way i see thing, you should never remove an option.

The Home Assistant view is that we should not try to please everyone because we will fail. Each option is extra complexity and overhead that should be avoided if almost nobody wants it anyway.

as requested by @amelchio, the need to enable telemetry to small to make thing more complicated.
@poofyteddy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

Then it's way easier :) i rollback to just putting the config needed to always opt-out of telemetry.

@amelchio amelchio dismissed their stale review Aug 29, 2019

Changed

Dev automation moved this from Needs review to Review in progress Aug 29, 2019
@amelchio

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

Then it's way easier :)

Exactly. And then we make less mistakes. Please remove the WIP label once you have tested this.

@poofyteddy poofyteddy changed the title WIP: Give user controle over Watson TTS Telemetry Give user controle over Watson TTS Telemetry Sep 3, 2019
@poofyteddy

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

After testing it it work as expected, i receive my audio file.
i could not do a end to end test since my tts player (squeezebox) don't seam to be willing to play the file, but if i download it with the same url i can play it on my computer without issue.

@amelchio

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

We need to get all CI checks to pass in order to move this along but I am not sure why some failed?

@frenck frenck added the docs-missing label Sep 12, 2019
@poofyteddy

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

hi @frenck, as far as i can tell, there isn't any doc to update since noting have change on the end ui.
https://www.home-assistant.io/components/watson_tts/
Sadly i don't have a dev env available, i'm editing it on github "ide" if you can call it that way. So i don't know why check are failing.

@frenck

This comment has been minimized.

Copy link
Member

commented Sep 12, 2019

@poofyteddy I've added it because you wrote TODO in your OP.

@poofyteddy

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

sorry @frenck my bad ... i didn't saw it when i check, and forgot about it when i removed the setting.
i really should let stuff like this to real dev :(

Thank's for the fix @amelchio 👍

@poofyteddy poofyteddy changed the title Give user controle over Watson TTS Telemetry Disable Watson TTS Telemetry Sep 12, 2019
@amelchio amelchio removed the docs-missing label Sep 12, 2019
Dev automation moved this from Review in progress to Reviewer approved Sep 12, 2019
@amelchio

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

Okay, let's merge this. Thanks for your contribution.

However, please do get a proper dev environment before making further PRs.

@amelchio amelchio merged commit 32a6a76 into home-assistant:dev Sep 12, 2019
11 checks passed
11 checks passed
CI Build #20190912.26 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing 33bd9c8...894bfc6
Details
codecov/project 93.99% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Sep 12, 2019
@lock lock bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
4 participants
You can’t perform that action at this time.