8000 Do not activate Met.no without setting a Home coordinates by frenck · Pull Request #48741 · home-assistant/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Do not activate 8000 Met.no without setting a Home coordinates #48741

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

Merged
merged 5 commits into from
Apr 7, 2021
Merged

Conversation

frenck
Copy link
Member
@frenck frenck commented Apr 6, 2021

Proposed change

Based on issue #48730, we do a lot of traffic to the Met.no APIs/servers based on the default location. This happens when a user doesn't set a location explicitly during onboarding and causes the core configuration to default to the example data from the frontend:

const amsterdam = [52.3731339, 4.8903147];

https://github.com/home-assistant/frontend/blob/109910d18f379e9a23cfc54f3f209e1d06042d72/src/onboarding/onboarding-core-config.ts#L29

Met.no is letting us know that we are in violation of their terms of services. This PR is to mitigate that. For more information see #48730.

Changes:

  • If during onboarding no location is set, abort the config flow entry creation. This prevents a configuration entry to be created in those cases.
  • For existing installations and configuration entries, this PR checks if we are tracking the Home location of the instance and if those coordinates are the default home location, if so, we log an error and abort the entry setup.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

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

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

8000
@probot-home-assistant
Copy link

Hey there @Danielhiversen, @thimic, mind taking a look at this pull request as its been labeled with an integration (met) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@pvizeli
Copy link
Member
pvizeli commented Apr 6, 2021

I think that is a frontend issue, we should not set some things somewhere. But I like the approach to abort it, should also work if we set 0.0 instead amsterdam

@frenck frenck marked this pull request as ready for review April 6, 2021 21:26
@frenck
Copy link
Member Author
frenck commented Apr 6, 2021

I agree this is a frontend issue at some level, however, we currently have a lot of these instances out there. So this is more or less mitigating raised issues/concerns.

Should I add 0.0 for the aborts as well?

Copy link
Member
@balloob balloob left a comment

Choose a reason for hiding this comment

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

This is an ok stop-gap measure for now. Let's get it out asap. We will work on more properly skipping setting a location .

@balloob
Copy link
Member
balloob commented Apr 6, 2021

Yes, let's also check on 0,0.

Copy link
Member
@Danielhiversen Danielhiversen left a comment

Choose a reason for hiding this comment

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

Tested, and seems to work correct here 👍

@frenck frenck merged commit 6ec8e17 into dev Apr 7, 2021
@frenck frenck deleted the frenck-2021-1079 branch April 7, 2021 07:39
@github-actions github-actions bot locked and limited conversation to collaborators Apr 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0