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 Glances sensors dynamically #28639

Merged
merged 10 commits into from Feb 10, 2020
Merged

Conversation

@engrbm87
Copy link
Contributor

engrbm87 commented Nov 8, 2019

Breaking Change:

Glances sensors are now dynamically added. Especially sensors for mounted disks and temperature sensors.

Description:

This PR adds Glances sensors dynamically based on the collected data from glances_api.
Also unsubscribe from signal updates when entity is disabled.

Related issue (if applicable): fixes #28480, fixes #28441

Pull request with documentation for home-assistant.io (if applicable): home-assistant/home-assistant.io#11352

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 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. Update and include derived files by running python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
@probot-home-assistant

This comment has been minimized.

Copy link

probot-home-assistant bot commented Nov 8, 2019

Hey there @fabaff, mind taking a look at this pull request as its been labeled with a integration (glances) you are listed as a codeowner for? Thanks!

"docker_active": ["Containers active", "", "mdi:docker"],
"docker_cpu_use": ["Containers CPU used", "%", "mdi:docker"],
"docker_memory_use": ["Containers RAM used", "MiB", "mdi:docker"],
}
TEMP_SENSORS = {"cpu_temp": ["CPU Temp", TEMP_CELSIUS, "mdi:thermometer"]}
CPU_TEMP = "cpu_temp"

This comment has been minimized.

Copy link
@fabaff

fabaff Nov 13, 2019

Member

I don't see a benefit to break it out of the existing dict in a new one.

This comment has been minimized.

Copy link
@engrbm87

engrbm87 Nov 13, 2019

Author Contributor

I broke it out because I don't want to create only one sensor for this type. I want to create multiple sensors.

This comment has been minimized.

Copy link
@fabaff

fabaff Nov 16, 2019

Member

You are doing a lookup for every sensor that will be created thus it doesn't matter if you stick with SENSOR_TYPES or use the new TEMP_SENSORS dict. The lookup is performed for the same key but for different dicts.

This comment has been minimized.

Copy link
@engrbm87

engrbm87 Nov 19, 2019

Author Contributor

I have updated the code so that sensors are added dynamically based on the collected data from glances_api.
For fs in case of multiple detected disks all will be added.
For sensors also all sensors are added.
For sensor_types whose key doesn't have values the sensor is not added at all.

for sensor in glances_data.api.data["sensors"]:
dev.append(
GlancesSensor(
glances_data, name, sensor["label"], CPU_TEMP, TEMP_SENSORS[CPU_TEMP]

This comment has been minimized.

Copy link
@fabaff

fabaff Nov 13, 2019

Member

We should adjust the naming as we are no longer only working with CPU temperatures but chipsets and other peripheral hardware.

This comment has been minimized.

Copy link
@engrbm87

engrbm87 Nov 13, 2019

Author Contributor

Ok, I can change this to sensor_temp. Or do you recommend another naming?

This comment has been minimized.

Copy link
@fabaff

fabaff Nov 16, 2019

Member

I'm fine with sensor_temp.

Dev automation moved this from Needs review to Review in progress Nov 13, 2019
@engrbm87 engrbm87 changed the title Add temp_sensors to glances dynamically Add Glances sensors dynamically Nov 19, 2019
engrbm87 added 2 commits Nov 21, 2019
@fabaff

This comment has been minimized.

Copy link
Member

fabaff commented Nov 28, 2019

The docs should be updated as well. I think that Integration Entities
should be adjusted or maybe better removed.

Now, it looks like a "breaking change".

@engrbm87 engrbm87 mentioned this pull request Dec 3, 2019
2 of 2 tasks complete
@frenck frenck requested a review from fabaff Jan 10, 2020
GlancesSensor(
client,
name,
disk["mnt_point"],

This comment has been minimized.

Copy link
@balloob

balloob Jan 15, 2020

Member

Can we somehow also put this in SENSOR_TYPES ? Maybe a lambda with extract info: lambda item: item["mnt_point"]

This comment has been minimized.

Copy link
@engrbm87

engrbm87 Feb 5, 2020

Author Contributor

I need to keep it as perfix because i use the prefix value when updating the entity in async_update

engrbm87 added 4 commits Jan 31, 2020
…into glances-temp-sensors
…into glances-temp-sensors
@engrbm87 engrbm87 mentioned this pull request Feb 7, 2020
3 of 7 tasks complete
@balloob balloob merged commit c66106e into home-assistant:dev Feb 10, 2020
10 checks passed
10 checks passed
CI Build #20200207.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 Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch 95.53% of diff hit (target 94.64%)
Details
codecov/project 94.65% (target 90%)
Details
Dev automation moved this from Review in progress to Done Feb 10, 2020
@engrbm87 engrbm87 mentioned this pull request Feb 11, 2020
2 of 7 tasks complete
@lock lock bot locked and limited conversation to collaborators Feb 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.