Update Sonarr config flow to standardize ports #127625
Conversation
home-assistant
bot
added
bugfix
cla-signed
code-quality
Hacktoberfest
has-tests
integration: sonarr
small-pr
|
Hey there @ctalkington, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
Khabi
changed the title
| @@ -5,6 +5,6 @@ | |||
| MOCK_REAUTH_INPUT = {CONF_API_KEY: "test-api-key-reauth"} | |||
|
|
|||
| MOCK_USER_INPUT = { | |||
| CONF_URL: "http://192.168.1.189:8989", | |||
| CONF_URL: "http://192.168.1.189:8989/", | |||
There was a problem hiding this comment.
My assumption is that at least a few users will probably have this proxied behind some kind of service an may have to have a path component, if no path is specified then yarl just does a / in place of it.
>>> url = yarl.URL("https://my.exmaple.com")
>>> url.path
'/'
| async def test_url_rewrite( | ||
| hass: HomeAssistant, | ||
| mock_sonarr_config_flow: MagicMock, | ||
| mock_setup_entry: None, | ||
| ) -> None: | ||
| """Test the full manual user flow from start to finish.""" | ||
| result = await hass.config_entries.flow.async_init( | ||
| DOMAIN, | ||
| context={CONF_SOURCE: SOURCE_USER}, | ||
| ) | ||
|
|
||
| assert result["type"] is FlowResultType.FORM | ||
| assert result["step_id"] == "user" | ||
|
|
||
| user_input = MOCK_USER_INPUT.copy() | ||
| user_input[CONF_URL] = "https://192.168.1.189" | ||
| result = await hass.config_entries.flow.async_configure( | ||
| result["flow_id"], | ||
| user_input=user_input, | ||
| ) | ||
|
|
||
| assert result["type"] is FlowResultType.CREATE_ENTRY | ||
| assert result["title"] == "192.168.1.189" | ||
|
|
||
| assert result["data"] | ||
| assert result["data"][CONF_URL] == "https://192.168.1.189:443/" |
There was a problem hiding this comment.
Should we parametrize this test and just throw a lot of scenarios to it so we are sure this keeps working like we expect? (We have seen some YARL issues in the last. weeks, so it would be useful to avoid regressions)
There was a problem hiding this comment.
I did notice when I added this to the radarr component that the tests were a little more rounded out. I'm not opposed to it, but might be a little bit before I have the time to sit down and knock that out.
There was a problem hiding this comment.
In the mean time, this is already an improvement, so let's move forward with this one at least.
Co-authored-by: Joost Lekkerkerker <[email protected]>
frenck
added
the
smash
Breaking change
Proposed change
The underlying
aiopyarrlibrary fills in the default configured application port (8989) if you don't explicitly specify the port. So when configuring the flow if you puthttps://sonarr.example.comit becomeshttps://sonarr.example.com:8989. This has caused issues such as #72372. This adds the correct default port based on the URL schema. In the example above it rewrites tohttps://sonarr.example.com:443as one would expect.Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: