Only raise integrationnotfound for dependencies

[alandtse]

Breaking change

Proposed change

Change the behavior of integrationnotfound to only raise for dependencies and not after_dependencies.

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:

# Example configuration.yaml

Additional information

• This PR fixes or closes issue: fixes Missing after_dependencies should not raise exception if dependency is missing #48200
• This PR is related to issue:
• Link to documentation pull request:

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:

• Documentation added/updated for www.home-assistant.io

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:

• I have reviewed two other open pull requests in this repository.

[bdraco]

One small suggestion above

[bdraco]

Thanks 👍

[frenck]

Should we only allow this behavior on custom integrations?

As for our core integrations, should not depend on external or non-existing ones (no matter if after or not).

[balloob]

I agree. We should only accept this behavior for custom components 👍

[bdraco]

Please make this only apply to non-core integrations per the discussion above

[alandtse]

Then shouldn't we block after dependencies for core components since it's the exact same as dependencies? The only distinction was the optionality.

[frenck]

Then shouldn't we block after dependencies for core components since it's the exact same as dependencies? The only distinction was the optionality.

The difference is the allowing to silently fail on non-existing after dependencies (non-existing integrations). In your linked issue, hacs is an after dependency. A user doesn't have to have that. For a custom integration, that is fine. However, we don't want to have loose dependencies in our core integration. If a core integration has an after dependency to hacs and a user doesn't have it... it should fail (just like before this PR).

[alandtse]

But that's not what the described behavior in the documentation is. If core treats both dependencies the same, it's not a useful field for core integrations to have after dependencies because it's not optional. They will fail if the after dependency is missing.

In theory a core integration could enable extra functionality if a custom component was available. That's what the field apparently was supposed to allow. If the core component should fail if that optional dependency is missing, the field should be removed for core.

[alandtse]

Specifically:

This option is used to specify dependencies that might be used by the integration but aren't essential. . . . For example, if the camera integration might use the stream integration in certain configurations, adding stream to after_dependencies of camera's manifest, will ensure that stream is loaded before camera if it is configured. If stream is not configured, camera will still load.

https://developers.home-assistant.io/docs/creating_integration_manifest/#after-dependencies

If after_dependencies should not have this behavior for core, it should be explicitly mentioned as not allowed by core. Otherwise, it behaves identically to dependencies and is a redundant option that isn't obvious to a developer. Personally, I think it should allow after_dependencies because it's possible that a core integration could leverage a custom integration. I don't think custom integrations should be treated as second class but that's another discussion.

[frenck]

Yes, this requires a change to the developer documentation as well. Nevertheless, it should be implemented as suggested above.

because it's possible that a core integration could leverage a custom integration

No, we should not allow that, that was the whole point of the review.

[alandtse]

Yes, but why are we offering two options that behave identically for core components? Shouldn't after_dependencies be marked as not allowed for core?

Again, I'm not arguing against implementation, I'm arguing that the existence of two identical options in manifest.json is confusing. I haven't heard a response on that point.

[frenck]

Yes, but why are we offering two options that behave identically for core components? Shouldn't after_dependencies be marked as not allowed for core?

Nope, for example, an integration can use cloud webhooks, but if a user doesn't have the cloud integration, then that is fine too. So within the core, this is actually used and needed.

[alandtse]

Change implemented and clarification to docs submitted.

[bdraco]

Lgtm once the pylint is fixed