Skip to content

Disable Watson TTS Telemetry#26253

Merged
amelchio merged 5 commits into
home-assistant:devfrom
poofyteddy:dev
Sep 12, 2019
Merged

Disable Watson TTS Telemetry#26253
amelchio merged 5 commits into
home-assistant:devfrom
poofyteddy:dev

Conversation

@poofyteddy
Copy link
Copy Markdown
Contributor

@poofyteddy poofyteddy 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.

This should do it from what i understand
@homeassistant
Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

@amelchio amelchio left a comment

Choose a reason for hiding this comment

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

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

Comment thread homeassistant/components/watson_tts/tts.py Outdated
@homeassistant
Copy link
Copy Markdown
Contributor

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!

@amelchio
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor Author

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 August 29, 2019 19:18

Changed

@amelchio
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Contributor

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

@poofyteddy
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Member

frenck commented Sep 12, 2019

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

@poofyteddy
Copy link
Copy Markdown
Contributor Author

poofyteddy 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
Copy link
Copy Markdown
Contributor

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
@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.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants