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 initial version of Vilfo Router integration #31177

Merged
merged 10 commits into from
Feb 12, 2020

Conversation

ManneW
Copy link
Contributor

@ManneW ManneW commented Jan 26, 2020

Proposed change

New integration added for the Vilfo Router (https://www.vilfo.com). The Vilfo Router is a VPN router with an HTTP API. In this initial version, the integration only provides a couple of sensors to monitor the status of the router. The idea is to add device tracking soon after this initial version (but as the guidelines states, this first version is limited to a single platform).

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

Example entry for configuration.yaml:

Integration only supports config_flow configuration.

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)

For the config_flow part of the integration:

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

@homeassistant
Copy link
Contributor

Hi @ManneW,

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!

@Quentame
Copy link
Member

Sorry for the spam I made a mistake

@ManneW
Copy link
Contributor Author

ManneW commented Jan 27, 2020

No worries! Were you trying to fix the cla-error label? I think that it can be removed now anyhow 🙂

@Quentame
Copy link
Member

Yeppa, I don't know why you have this now (also I was on a mobile app, and when I've add cla-recheck removed all other tags).

The recheck is not removing it. I'll let people who knows taking take of it.

Or maybe "ManneW authored and Emanuel Winblad committed" on the first commit, try rebase and force push, so it will be you the author and committer.

So, I'll review soon 😊

@ManneW
Copy link
Contributor Author

ManneW commented Jan 27, 2020

I actually did amend the "author" part to resolve the CLA issue from the beginning (before that it was only "Emanuel Winblad" and not "ManneW") 😄

But your suggestion was a good idea, so I did the rebase now to have things a bit cleaner 🙂Not sure if the cla-error label will go away though, but we'll see.

Copy link
Member

@Quentame Quentame left a comment

Choose a reason for hiding this comment

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

Why there is a src/vilfo-api-client submodule here ?
It shouldn't be.

homeassistant/components/vilfo/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/vilfo/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/vilfo/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/vilfo/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/vilfo/__init__.py Show resolved Hide resolved
homeassistant/components/vilfo/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/vilfo/const.py Outdated Show resolved Hide resolved
homeassistant/components/vilfo/sensor.py Outdated Show resolved Hide resolved
tests/components/vilfo/test_config_flow.py Outdated Show resolved Hide resolved
homeassistant/components/vilfo/config_flow.py Outdated Show resolved Hide resolved
* Removed unit of measurement for boot time sensor,
* Added support for a sensor not having a unit,
* Updated the config_flow to handle when the integration is already configured,
* Updated tests to avoid mocking the code under test and also to cover the aforementioned changes.
@ManneW
Copy link
Contributor Author

ManneW commented Feb 12, 2020

I've updated (but not pushed yet) this based on the last comments from @MartinHjelmare. Do you want me to squash all the work into a single commit?

I'll push it as a single new commit in the meantime, but are ready to do some squashing to keep the repo as clean as possible 😄

@MartinHjelmare
Copy link
Member

Please don't squash commits after review has started to make it easier for readers to track changes.

We'll squash when merging. 👍

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Very nice!

@MartinHjelmare
Copy link
Member

Can be merged when build passes.

@ManneW
Copy link
Contributor Author

ManneW commented Feb 12, 2020

Thank you for great review feedback, @MartinHjelmare and @Quentame! 🙏

No worries, @Quentame! 😄 I guess it would be good if you also marked your original review as "approved" (or if one can remove the review or something?) so that the PR can be sorted accordingly 🙂

@ManneW ManneW requested review from Quentame and removed request for Quentame February 12, 2020 13:26
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Sorry, I think I've missed this.

homeassistant/components/vilfo/__init__.py Outdated Show resolved Hide resolved
homeassistant/components/vilfo/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@Quentame Quentame left a comment

Choose a reason for hiding this comment

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

3 comments more before merging

homeassistant/components/vilfo/const.py Outdated Show resolved Hide resolved
homeassistant/components/vilfo/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/vilfo/sensor.py Show resolved Hide resolved
@codecov

This comment has been minimized.

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Thanks!

@Quentame Quentame merged commit 3e05fc1 into home-assistant:dev Feb 12, 2020
@Quentame
Copy link
Member

Very nice work @ManneW !

Thanks for contributing 👏

@lock lock bot locked and limited conversation to collaborators Feb 13, 2020
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.

4 participants