Skip to content

tapo: drop IP-address handling, use host string throughout#29914

Draft
andig wants to merge 2 commits into
masterfrom
tapo-host-cleanup
Draft

tapo: drop IP-address handling, use host string throughout#29914
andig wants to merge 2 commits into
masterfrom
tapo-host-cleanup

Conversation

@andig
Copy link
Copy Markdown
Member

@andig andig commented May 15, 2026

Summary

Cleanup follow-up to #29768. Hands the configured URI's host straight to the Tapo library instead of round-tripping through netip.Addr.

  • removes the netip.ParseAddr step (and the "invalid ip address" rejection of hostnames)
  • consolidates URL parsing into one step
  • no DNS resolution lives in evcc anymore — the standard library resolver handles it via net/http inside the tapo library

Companion upstream PR: insomniacslk/tapo#8 (drops netip.Addr from the public API; tapo.NewPlug now takes a host string).

Replace #29768

The Tapo library used to require a netip.Addr, which forced us to parse the
configured URI, extract the hostname, and reject anything that wasn't an IP
literal. With insomniacslk/tapo#8 the library now takes a plain host string
(hostname or IP), so we can hand the URL's Host straight through.

This makes the UI's "host or IP" hint actually work, no DNS resolution lives
in evcc anymore, and the connection setup is one step shorter.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • Consider whether tapo.NewPlug should receive u.Hostname() instead of u.Host, as the current change will now pass through any explicit port as part of the host string, which differs from the previous netip.ParseAddr(url.Hostname()) behavior.
  • The NewConnection URI validation only checks u.Host == ""; if you care about rejecting malformed inputs like missing scheme or path-only strings, you might want to use url.ParseRequestURI or add an explicit scheme/host sanity check.
  • Given the temporary replace directive in go.mod, you might want to add a brief inline comment (e.g. // TODO: drop once insomniacslk/tapo#8 is tagged) to make the intent clear to future maintainers.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Consider whether `tapo.NewPlug` should receive `u.Hostname()` instead of `u.Host`, as the current change will now pass through any explicit port as part of the host string, which differs from the previous `netip.ParseAddr(url.Hostname())` behavior.
- The `NewConnection` URI validation only checks `u.Host == ""`; if you care about rejecting malformed inputs like missing scheme or path-only strings, you might want to use `url.ParseRequestURI` or add an explicit scheme/host sanity check.
- Given the temporary `replace` directive in `go.mod`, you might want to add a brief inline comment (e.g. `// TODO: drop once insomniacslk/tapo#8 is tagged`) to make the intent clear to future maintainers.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

With the upstream library accepting a plain host string, there is no need
for evcc to round-trip through a URL. Switch the config field and the
template render output from "uri: http://{{ .host }}" to "host: {{ .host }}",
drop the url package import in connection.go, and pass the host directly
to tapo.NewPlug.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@andig andig added the devices Specific device support label May 15, 2026
@andig andig marked this pull request as draft May 15, 2026 15:31
@github-actions github-actions Bot added the stale Outdated and ready to close label May 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

devices Specific device support stale Outdated and ready to close

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant