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

Fix update on cert_expiry startup #27137

Merged
merged 6 commits into from Oct 3, 2019

Conversation

@jjlawren
Copy link
Contributor

commented Oct 3, 2019

Description:

Cert Expiry sensors are now updated when the entities are created if home assistant is running or at home assistant start if home assistant is not running yet.

Import of config yaml is delayed and done at home assistant start, to avoid issues with checking certificate served by home assistant.

Related issue (if applicable): may fix #26740

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
@jjlawren

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2019

I'm guessing the receiving server didn't like to accept multiple connections from the same source in quick succession, rejecting the new connections and causing the entity to fall into a bad state. It sounds like they updated fine at the next 12 hour update.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

Yeah I think we shouldn't update twice close like we do now. But which update should we remove?

@jjlawren

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2019

The self-connecting scenario is one I hadn't considered. Probably should swap back to updating when HA is fully started. I'll do that shortly.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

Yeah. Let's create the entity as available and remove the update during entity addition. Keep the update on home assistant start.

The entity will then be created with unknown state. When the first update happens, either on home assistant start or 12 hours after creating the entity, the entity will get correct state.

I guess we could add some more complex logic in async_added_to_hass that checks the run state of home assistant. If it's already running, we schedule an update immediately. If home assistant is setting up, we schedule an update on home assistant start event.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

To solve the problem with importing the config yaml, we could schedule the import on home assistant start event.

@frenck frenck added the Hacktoberfest label Oct 3, 2019
Copy link
Member

left a comment

Nice!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Oct 3, 2019

Do you want to change config yaml import scheduling in a separate PR?

@jjlawren

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2019

I removed that since it didn't seem necessary anymore with the conditional updates when the entities are added. Do you think something is still needed there?

@jjlawren

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2019

You're right, I forgot the import would attempt to validate the configuration. If it was checking itself this would cause the import to fail. Updated the import step to delay until HA is fully started.

Copy link
Member

left a comment

Great!

@MartinHjelmare MartinHjelmare changed the title Don't force redundant update on cert_expiry startup Fix update on cert_expiry startup Oct 3, 2019
@MartinHjelmare MartinHjelmare merged commit bb45bdd into home-assistant:dev Oct 3, 2019
9 checks passed
9 checks passed
CI Build #20191003.33 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 Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
@jjlawren jjlawren deleted the jjlawren:cert_update_once branch Oct 3, 2019
@lock lock bot locked and limited conversation to collaborators Oct 4, 2019
@balloob balloob added this to the 0.100 milestone Oct 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.