Skip to content

Remove colorlog dep if available#20272

Closed
kellerza wants to merge 1 commit into
home-assistant:devfrom
kellerza:config_validation_colorlog
Closed

Remove colorlog dep if available#20272
kellerza wants to merge 1 commit into
home-assistant:devfrom
kellerza:config_validation_colorlog

Conversation

@kellerza
Copy link
Copy Markdown
Member

Description:

Removed colorlog REQUIREMENT if this is already present
There are multiple reports of colorlog installing everytime on config_check execution — I am seeing this in the dev docker image as well

Another approach would be to make this part of the core requirements... (would prefer this to be honest)

Related issue (if applicable): fixes #18651

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#<home-assistant.io PR number goes here>

Example entry for configuration.yaml (if applicable):

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.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@homeassistant homeassistant added cla-signed core small-pr PRs with less than 30 lines. labels Jan 20, 2019
@ghost ghost assigned kellerza Jan 20, 2019
@ghost ghost added the in progress label Jan 20, 2019
try:
import colorlog.escape_codes # noqa # pylint: disable=unused-import
except ImportError:
REQUIREMENTS = ('colorlog==4.0.2',)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will confuse our requirement generator script.

The dev docker image should already have colorlog, so should not install it every time?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Indeed, it's not added, but there might also be a problem with how we detect colorlog inside Docker If I read this correctly?

root@linuxkit-00155d5dc21f:/usr/src/app# pip list | grep colorlog
colorlog                          4.0.2
root@linuxkit-00155d5dc21f:/usr/src/app# hass --script check_config --config /config
INFO:homeassistant.util.package:Attempting install of colorlog==4.0.2
Testing configuration at /config
Failed config
  homeassistant:
    - extra keys not allowed @ data['packages']['yr__pack']

Another approach would be to make this part of the core requirements... (would prefer this to be honest)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We shouldn't merge this. We're breaking something because some other wrapper or runner doesn't install correctly or keep it around.

This is not something that should be fixed inside Home Assistant as it works as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed core small-pr PRs with less than 30 lines.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Trying to install color-log every time config check is run in CLI

3 participants