Fix librouteros login_method argument#489
Closed
alvarodgarcia wants to merge 1 commit into
Closed
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdate Mikrotik Router integration to use the new librouteros Sequence diagram for MikrotikApi connect using new librouteros login_methodsequenceDiagram
actor HomeAssistant
participant MikrotikApi
participant librouteros
participant librouteros_login as librouteros_login_module
HomeAssistant->>MikrotikApi: connect()
MikrotikApi->>MikrotikApi: read _login_method
alt login_method is plain string
MikrotikApi->>librouteros_login: resolve plain
librouteros_login-->>MikrotikApi: plain function
else login_method is token string
MikrotikApi->>librouteros_login: resolve token
librouteros_login-->>MikrotikApi: token function
else login_method already function
MikrotikApi->>MikrotikApi: use existing callable
end
MikrotikApi->>librouteros: connect(encoding, login_method, port)
librouteros-->>MikrotikApi: connection
MikrotikApi-->>HomeAssistant: connection established
Class diagram for MikrotikApi connect login_method handlingclassDiagram
class MikrotikApi {
- _encoding
- _login_method
- _port
- _connected
- _connection_epoch
+ connect() bool
}
class librouteros_login {
+ plain
+ token
}
class librouteros_client_factory {
+ connect(encoding, login_method, port)
}
MikrotikApi ..> librouteros_login : uses
MikrotikApi ..> librouteros_client_factory : calls connect with login_method
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider handling unexpected
self._login_methodvalues (e.g., logging a warning or falling back to a default) so misconfigurations don’t silently result in unusablelogin_methodarguments. - If
self._login_methodcan already be a callable (e.g., set elsewhere in code), you may want to skip the string-to-function mapping when it’s not a string to avoid unnecessarily overriding a valid login function. - You might want to normalize/resolve
self._login_methodto the appropriate function once at initialization or configuration time instead of on everyconnect()call to keep connection logic simpler and avoid repeated mapping.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider handling unexpected `self._login_method` values (e.g., logging a warning or falling back to a default) so misconfigurations don’t silently result in unusable `login_method` arguments.
- If `self._login_method` can already be a callable (e.g., set elsewhere in code), you may want to skip the string-to-function mapping when it’s not a string to avoid unnecessarily overriding a valid login function.
- You might want to normalize/resolve `self._login_method` to the appropriate function once at initialization or configuration time instead of on every `connect()` call to keep connection logic simpler and avoid repeated mapping.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
85e67fb to
52ffd91
Compare
carpenike
added a commit
to carpenike/homeassistant-mikrotik_router
that referenced
this pull request
May 7, 2026
librouteros 4.0 (released 2026-04) made two breaking changes that prevent
this integration from connecting:
1. The 'login_methods' (plural) keyword argument was renamed to
'login_method' (singular). Old code raises:
connect() got an unexpected keyword argument 'login_methods'.
Did you mean 'login_method'?
2. The value must now be a callable (e.g. librouteros.login.plain), not
the legacy string 'plain'. Old code raises:
'str' object is not callable
Anyone with a fresh HA environment will resolve librouteros 4.x via pip
because our requirement is '>=3.4.1' with no upper bound. Without this
fix the integration is unusable on those installs.
Fix (adapted from upstream PR tomaae#489 by @alvarodgarcia):
- Import librouteros.login.plain and librouteros.login.token at module
load (these symbols also exist in librouteros 3.x).
- In connect(), if self._login_method is still a string from saved
config, translate 'plain'/'token' to the matching callable. Unknown
string values log a warning and fall back to plain. Already-callable
values pass through unchanged.
- Rename the kwarg to 'login_method' (singular) — the canonical name
in both librouteros 3.x and 4.x.
DEFAULT_LOGIN_METHOD in const.py is left as the string 'plain' on
purpose: it is what gets persisted into existing config entries, so
keeping the string + runtime translation preserves backward-compat for
all existing installs without a migration.
|
This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
login_methodstologin_method.plain/token) to the functions expected by newerlibrouteros.Why
Newer
librouterosversions reject the oldlogin_methodskeyword with:This prevents the integration from connecting after Home Assistant loads a newer
librouterosin the environment.Validation
librouterosexposinglogin_method.python3 -m py_compile custom_components/mikrotik_router/mikrotikapi.py.