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

Add notes about the Google Assistant integration being broken #8076

Open
wants to merge 16 commits into
base: dev
Choose a base branch
from

Conversation

inventor96
Copy link
Contributor

No description provided.

@bewest
Copy link
Member

bewest commented Aug 16, 2023

Thanks for confirming, should we remove the code and log a deprecation warning as well?

@inventor96
Copy link
Contributor Author

We could, but I'm wondering if it might be helpful to keep it in case we can revive it again, or if someone figures out a good way to use one of Google's alternatives to Conversational Actions. As far as I'm aware, it doesn't expose any security vulnerabilities, and it doesn't add overhead to the application.

The code that supports Google Assistant specifically is just a "translator" that sits between the Google APIs and the generic virtual assistant code in Nightscout. The Alexa integration also depends on the generic part of the code, so we at least don't want to remove that part for sure.

@bewest
Copy link
Member

bewest commented Aug 16, 2023

I'm interested in otherwise comparable alternatives. To the extent we can remove any dependencies, we would like to do so, especially non-functional ones. We've spent the better part of the last year working on bringing dependencies up to date and to prepare for removing things that aren't going to help us move forward. It makes sense to me to remove all the google-assistant specific code and replace it with a warning on the console, especially if it allows us to remove a dependency, especially any additional http libraries.

@inventor96
Copy link
Contributor Author

Good to know. Thankfully there's no dependencies

@inventor96 inventor96 closed this Aug 16, 2023
@inventor96
Copy link
Contributor Author

Whoops, wrong button.

There's no dependencies that are unique to the Google Assistant integration, so we're clear there. I suppose we could remove the code, and maybe I'll stick a link in the docs to the last commit with the code. That way anyone who figures out a good way to do it again can use it as a reference.

@inventor96 inventor96 reopened this Aug 16, 2023
@inventor96
Copy link
Contributor Author

In what context do you think a warning log message will be needed? Like if they have the plugin in the enabled list?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants