fix(inputs.huebridge): Cleanup and fix linter issues (#16606)

2025-03-10 16:56:51 +01:00 · 2025-03-10 16:56:51 +01:00 · 8974563967
parent 0001ae490a
commit 8974563967
5 changed files with 153 additions and 131 deletions
--- a/docs/LICENSE_OF_DEPENDENCIES.md
+++ b/docs/LICENSE_OF_DEPENDENCIES.md
@ -347,7 +347,6 @@ following works:
 - github.com/rivo/uniseg [MIT License](https://github.com/rivo/uniseg/blob/master/LICENSE.txt)
 - github.com/robbiet480/go.nut [MIT License](https://github.com/robbiet480/go.nut/blob/master/LICENSE)
 - github.com/robinson/gos7 [BSD 3-Clause "New" or "Revised" License](https://github.com/robinson/gos7/blob/master/LICENSE)
- github.com/rs/zerolog [MIT License](https://github.com/rs/zerolog/blob/master/LICENSE)
 - github.com/russross/blackfriday [BSD 2-Clause "Simplified" License](https://github.com/russross/blackfriday/blob/master/LICENSE.txt)
 - github.com/safchain/ethtool [Apache License 2.0](https://github.com/safchain/ethtool/blob/master/LICENSE)
 - github.com/samber/lo [MIT License](https://github.com/samber/lo/blob/master/LICENSE)
@ -452,7 +451,6 @@ following works:
 - gopkg.in/gorethink/gorethink.v3 [Apache License 2.0](https://github.com/rethinkdb/rethinkdb-go/blob/v3.0.5/LICENSE)
 - gopkg.in/inf.v0 [BSD 3-Clause "New" or "Revised" License](https://github.com/go-inf/inf/blob/v0.9.1/LICENSE)
 - gopkg.in/ini.v1 [Apache License 2.0](https://github.com/go-ini/ini/blob/master/LICENSE)
- gopkg.in/natefinch/lumberjack.v2 [MIT License](https://github.com/natefinch/lumberjack/blob/v2.2.1/LICENSE)
 - gopkg.in/olivere/elastic.v5 [MIT License](https://github.com/olivere/elastic/blob/v5.0.76/LICENSE)
 - gopkg.in/tomb.v1 [BSD 3-Clause Clear License](https://github.com/go-tomb/tomb/blob/v1/LICENSE)
 - gopkg.in/tomb.v2 [BSD 3-Clause Clear License](https://github.com/go-tomb/tomb/blob/v2/LICENSE)
--- a/plugins/inputs/huebridge/README.md
+++ b/plugins/inputs/huebridge/README.md
@ -1,7 +1,7 @@
 # HueBridge Input Plugin

-This input plugin gathers status from [Hue Bridge][hue] devices
-using the [CLIP API][hue_api] interface of the devices.
+This plugin gathers status from [Hue Bridge][hue] devices using the
+[CLIP API][hue_api] interface of the devices.

 ⭐ Telegraf v1.34.0
 🏷️ iot
--- a/plugins/inputs/huebridge/bridge.go
+++ b/plugins/inputs/huebridge/bridge.go
@ -1,6 +1,7 @@
 package huebridge

 import (
+	"crypto/tls"
 	"fmt"
 	"maps"
 	"math"
@ -11,55 +12,23 @@ import (
 	"strings"
 	"time"

-	"github.com/influxdata/telegraf"
-	"github.com/influxdata/telegraf/config"
-	"github.com/influxdata/telegraf/plugins/common/tls"
 	"github.com/tdrn-org/go-hue"
+
+	"github.com/influxdata/telegraf"
 )

 type bridge struct {
 	url                   *url.URL
 	configRoomAssignments map[string]string
-	rcc                   *RemoteClientConfig
-	tcc                   *tls.ClientConfig
-	timeout               config.Duration
+	remoteCfg             *RemoteClientConfig
+	tlsCfg                *tls.Config
+	timeout               time.Duration
 	log                   telegraf.Logger
-	resolvedClient        hue.BridgeClient
-	resourceTree          map[string]string
-	deviceNames           map[string]string
-	roomAssignments       map[string]string
-}

-func newBridge(rawUrl string, roomAssignments map[string]string, rcc *RemoteClientConfig, tcc *tls.ClientConfig, timeout config.Duration, log telegraf.Logger) (*bridge, error) {
-	parsedUrl, err := url.Parse(rawUrl)
-	if err != nil {
-		return nil, fmt.Errorf("failed to parse bridge URL %s: %w", rawUrl, err)
-	}
-	switch parsedUrl.Scheme {
-	case "address", "cloud", "mdns", "remote":
-		// Do nothing, those are valid
-	default:
-		return nil, fmt.Errorf("unrecognized scheme %s in URL %s", parsedUrl.Scheme, parsedUrl)
-	}
-	// All schemes require a password in the URL
-	_, passwordSet := parsedUrl.User.Password()
-	if !passwordSet {
-		return nil, fmt.Errorf("missing password in URL %s", parsedUrl)
-	}
-	// Remote scheme also requires a configured rcc
-	if parsedUrl.Scheme == "remote" {
-		if rcc.RemoteClientId == "" || rcc.RemoteClientSecret == "" || rcc.RemoteTokenDir == "" {
-			return nil, fmt.Errorf("missing remote application credentials and/or token director not configured")
-		}
-	}
-	return &bridge{
-		url:                   parsedUrl,
-		configRoomAssignments: roomAssignments,
-		rcc:                   rcc,
-		tcc:                   tcc,
-		timeout:               timeout,
-		log:                   log,
-	}, nil
+	resolvedClient  hue.BridgeClient
+	resourceTree    map[string]string
+	deviceNames     map[string]string
+	roomAssignments map[string]string
 }

 func (b *bridge) String() string {
@ -73,8 +42,7 @@ func (b *bridge) process(acc telegraf.Accumulator) error {
 		}
 	}
 	b.log.Tracef("Processing bridge %s", b)
-	err := b.fetchMetadata()
-	if err != nil {
+	if err := b.fetchMetadata(); err != nil {
 		// Discard previously resolved client and re-resolve on next process call
 		b.resolvedClient = nil
 		return err
@ -250,18 +218,13 @@ func (b *bridge) resolveViaAddress() error {
 func (b *bridge) resolveViaCloud() error {
 	locator := hue.NewCloudBridgeLocator()
 	if b.url.Host != "" {
-		discoveryEndpointUrl, err := url.Parse(fmt.Sprintf("https://%s/", b.url.Host))
+		u, err := url.Parse(fmt.Sprintf("https://%s/", b.url.Host))
 		if err != nil {
 			return err
 		}
-		discoveryEndpointUrl = discoveryEndpointUrl.JoinPath(b.url.Path)
-		locator.DiscoveryEndpointUrl = discoveryEndpointUrl
+		locator.DiscoveryEndpointUrl = u.JoinPath(b.url.Path)
 	}
-	tlsConfig, err := b.tcc.TLSConfig()
-	if err != nil {
-		return err
-	}
-	locator.TlsConfig = tlsConfig
+	locator.TlsConfig = b.tlsCfg
 	return b.resolveLocalBridge(locator)
 }

@ -271,12 +234,12 @@ func (b *bridge) resolveViaMDNS() error {
 }

 func (b *bridge) resolveLocalBridge(locator hue.BridgeLocator) error {
-	hueBridge, err := locator.Lookup(b.url.User.Username(), time.Duration(b.timeout))
+	hueBridge, err := locator.Lookup(b.url.User.Username(), b.timeout)
 	if err != nil {
 		return err
 	}
 	urlPassword, _ := b.url.User.Password()
-	bridgeClient, err := hueBridge.NewClient(hue.NewLocalBridgeAuthenticator(urlPassword), time.Duration(b.timeout))
+	bridgeClient, err := hueBridge.NewClient(hue.NewLocalBridgeAuthenticator(urlPassword), b.timeout)
 	if err != nil {
 		return err
 	}
@ -285,42 +248,46 @@ func (b *bridge) resolveLocalBridge(locator hue.BridgeLocator) error {
 }

 func (b *bridge) resolveViaRemote() error {
-	var redirectUrl *url.URL
-	if b.rcc.RemoteCallbackUrl != "" {
-		parsedRedirectUrl, err := url.Parse(b.rcc.RemoteCallbackUrl)
+	var redirectURL *url.URL
+	if b.remoteCfg.RemoteCallbackURL != "" {
+		u, err := url.Parse(b.remoteCfg.RemoteCallbackURL)
 		if err != nil {
 			return err
 		}
-		redirectUrl = parsedRedirectUrl
+		redirectURL = u
 	}
-	tokenFile := filepath.Join(b.rcc.RemoteTokenDir, b.rcc.RemoteClientId, strings.ToUpper(b.url.User.Username())+".json")
-	locator, err := hue.NewRemoteBridgeLocator(b.rcc.RemoteClientId, b.rcc.RemoteClientSecret, redirectUrl, tokenFile)
+	tokenFile := filepath.Join(
+		b.remoteCfg.RemoteTokenDir,
+		b.remoteCfg.RemoteClientID,
+		strings.ToUpper(b.url.User.Username())+".json",
+	)
+	locator, err := hue.NewRemoteBridgeLocator(
+		b.remoteCfg.RemoteClientID,
+		b.remoteCfg.RemoteClientSecret,
+		redirectURL,
+		tokenFile,
+	)
 	if err != nil {
 		return err
 	}
 	if b.url.Host != "" {
-		endpointUrl, err := url.Parse(fmt.Sprintf("https://%s/", b.url.Host))
+		u, err := url.Parse(fmt.Sprintf("https://%s/", b.url.Host))
 		if err != nil {
 			return err
 		}
-		endpointUrl = endpointUrl.JoinPath(b.url.Path)
-		locator.EndpointUrl = endpointUrl
+		locator.EndpointUrl = u.JoinPath(b.url.Path)
 	}
-	tlsConfig, err := b.tcc.TLSConfig()
-	if err != nil {
-		return err
-	}
-	locator.TlsConfig = tlsConfig.Clone()
+	locator.TlsConfig = b.tlsCfg
 	return b.resolveRemoteBridge(locator)
 }

 func (b *bridge) resolveRemoteBridge(locator *hue.RemoteBridgeLocator) error {
-	hueBridge, err := locator.Lookup(b.url.User.Username(), time.Duration(b.timeout))
+	hueBridge, err := locator.Lookup(b.url.User.Username(), b.timeout)
 	if err != nil {
 		return err
 	}
 	urlPassword, _ := b.url.User.Password()
-	bridgeClient, err := hueBridge.NewClient(hue.NewRemoteBridgeAuthenticator(locator, urlPassword), time.Duration(b.timeout))
+	bridgeClient, err := hueBridge.NewClient(hue.NewRemoteBridgeAuthenticator(locator, urlPassword), b.timeout)
 	if err != nil {
 		return err
 	}
@ -405,7 +372,7 @@ func (b *bridge) fetchRoomAssignments() error {
 	return nil
 }

-func (b *bridge) resolveResourceRoom(resourceId string, resourceName string) string {
+func (b *bridge) resolveResourceRoom(resourceID, resourceName string) string {
 	roomName := b.roomAssignments[resourceName]
 	if roomName != "" {
 		return roomName
@ -414,15 +381,15 @@ func (b *bridge) resolveResourceRoom(resourceId string, resourceName string) str
 	// its owners until we find a room or there is no more owner. The latter
 	// may happen (e.g. for Motion Sensors) resulting in room name
 	// "<unassigned>".
-	currentResourceId := resourceId
+	currentResourceID := resourceID
 	for {
 		// Try next owner
-		currentResourceId = b.resourceTree[currentResourceId]
-		if currentResourceId == "" {
+		currentResourceID = b.resourceTree[currentResourceID]
+		if currentResourceID == "" {
 			// No owner left but no room found
 			break
 		}
-		roomName = b.roomAssignments[currentResourceId]
+		roomName = b.roomAssignments[currentResourceID]
 		if roomName != "" {
 			// Room name found, done
 			return roomName
@ -431,23 +398,23 @@ func (b *bridge) resolveResourceRoom(resourceId string, resourceName string) str
 	return "<unassigned>"
 }

-func (b *bridge) resolveDeviceName(resourceId string) string {
-	deviceName := b.deviceNames[resourceId]
+func (b *bridge) resolveDeviceName(resourceID string) string {
+	deviceName := b.deviceNames[resourceID]
 	if deviceName != "" {
 		return deviceName
 	}
 	// If resource does not have a device name assigned directly, iterate
 	// upwards via its owners until we find a room or there is no more
 	// owner. The latter may happen resulting in device name "<undefined>".
-	currentResourceId := resourceId
+	currentResourceID := resourceID
 	for {
 		// Try next owner
-		currentResourceId = b.resourceTree[currentResourceId]
-		if currentResourceId == "" {
+		currentResourceID = b.resourceTree[currentResourceID]
+		if currentResourceID == "" {
 			// No owner left but no device found
 			break
 		}
-		deviceName = b.deviceNames[currentResourceId]
+		deviceName = b.deviceNames[currentResourceID]
 		if deviceName != "" {
 			// Device name found, done
 			return deviceName
--- a/plugins/inputs/huebridge/huebridge.go
+++ b/plugins/inputs/huebridge/huebridge.go
@ -3,6 +3,9 @@ package huebridge

 import (
 	_ "embed"
+	"errors"
+	"fmt"
+	"net/url"
 	"sync"
 	"time"

@ -16,9 +19,9 @@ import (
 var sampleConfig string

 type RemoteClientConfig struct {
-	RemoteClientId     string `toml:"remote_client_id"`
+	RemoteClientID     string `toml:"remote_client_id"`
 	RemoteClientSecret string `toml:"remote_client_secret"`
-	RemoteCallbackUrl  string `toml:"remote_callback_url"`
+	RemoteCallbackURL  string `toml:"remote_callback_url"`
 	RemoteTokenDir     string `toml:"remote_token_dir"`
 }

@ -38,15 +41,45 @@ func (*HueBridge) SampleConfig() string {
 }

 func (h *HueBridge) Init() error {
-	h.bridges = make([]*bridge, 0, len(h.BridgeUrls))
-	for _, bridgeUrl := range h.BridgeUrls {
-		bridge, err := newBridge(bridgeUrl, h.RoomAssignments, &h.RemoteClientConfig, &h.ClientConfig, h.Timeout, h.Log)
-		if err != nil {
-			h.Log.Warnf("Failed to instantiate bridge for URL %s: %s", bridgeUrl, err)
-			continue
-		}
-		h.bridges = append(h.bridges, bridge)
+	tlsCfg, err := h.ClientConfig.TLSConfig()
+	if err != nil {
+		return fmt.Errorf("creating TLS configuration failed: %w", err)
 	}
+
+	h.bridges = make([]*bridge, 0, len(h.BridgeUrls))
+	for _, b := range h.BridgeUrls {
+		u, err := url.Parse(b)
+		if err != nil {
+			return fmt.Errorf("failed to parse bridge URL %s: %w", b, err)
+		}
+
+		switch u.Scheme {
+		case "address", "cloud", "mdns":
+			// Do nothing, those are valid
+		case "remote":
+			// Remote scheme also requires a configured rcc
+			if h.RemoteClientID == "" || h.RemoteClientSecret == "" || h.RemoteTokenDir == "" {
+				return errors.New("missing remote application credentials and/or token director not configured")
+			}
+		default:
+			return fmt.Errorf("unrecognized scheme %s in URL %s", u.Scheme, b)
+		}
+
+		// All schemes require a password in the URL
+		if _, set := u.User.Password(); !set {
+			return fmt.Errorf("missing password in URL %s", u)
+		}
+
+		h.bridges = append(h.bridges, &bridge{
+			url:                   u,
+			configRoomAssignments: h.RoomAssignments,
+			remoteCfg:             &h.RemoteClientConfig,
+			tlsCfg:                tlsCfg,
+			timeout:               time.Duration(h.Timeout),
+			log:                   h.Log,
+		})
+	}
+
 	return nil
 }

--- a/plugins/inputs/huebridge/huebridge_test.go
+++ b/plugins/inputs/huebridge/huebridge_test.go
@ -2,6 +2,7 @@ package huebridge

 import (
 	"fmt"
+	"path/filepath"
 	"testing"
 	"time"

@ -28,9 +29,9 @@ func TestConfig(t *testing.T) {

 	// Verify everything is setup according to config file
 	require.Len(t, h.BridgeUrls, 4)
-	require.Equal(t, "client", h.RemoteClientId)
+	require.Equal(t, "client", h.RemoteClientID)
 	require.Equal(t, "secret", h.RemoteClientSecret)
-	require.Equal(t, "url", h.RemoteCallbackUrl)
+	require.Equal(t, "url", h.RemoteCallbackURL)
 	require.Equal(t, "dir", h.RemoteTokenDir)
 	require.Len(t, h.RoomAssignments, 2)
 	require.Equal(t, config.Duration(60*time.Second), h.Timeout)
@ -48,7 +49,7 @@ func TestInitSuccess(t *testing.T) {
 			"remote://12345678:secret@localhost/",
 		},
 		RemoteClientConfig: RemoteClientConfig{
-			RemoteClientId:     mock.MockClientId,
+			RemoteClientID:     mock.MockClientId,
 			RemoteClientSecret: mock.MockClientSecret,
 			RemoteTokenDir:     ".",
 		},
@ -67,32 +68,71 @@ func TestInitSuccess(t *testing.T) {
 }

 func TestInitIgnoreInvalidUrls(t *testing.T) {
-	// The following URLs are all invalid must all be ignored during Init
-	h := &HueBridge{
-		BridgeUrls: []string{
-			"invalid://12345678:secret@invalid-scheme.net/",
-			"address://12345678@missing-password.net/",
-			"cloud://12345678@missing-password.net/",
-			"mdns://12345678@missing-password.net/",
-			"remote://12345678@missing-password.net/",
-			"remote://12345678:secret@missing-remote-config.net/",
+	tests := []struct {
+		addr     string
+		expected string
+	}{
+		{
+			addr:     "invalid://12345678:secret@invalid-scheme.net/",
+			expected: "unrecognized scheme",
+		},
+		{
+			addr:     "address://12345678@missing-password.net/",
+			expected: "missing password in URL",
+		},
+		{
+			addr:     "cloud://12345678@missing-password.net/",
+			expected: "missing password in URL",
+		},
+		{
+			addr:     "mdns://12345678@missing-password.net/",
+			expected: "missing password in URL",
+		},
+		{
+			addr:     "remote://12345678@missing-password.net/",
+			expected: "missing remote application credentials and/or token director not configured",
+		},
+		{
+			addr:     "remote://12345678:secret@missing-remote-config.net/",
+			expected: "missing remote application credentials and/or token director not configured",
 		},
-		Timeout: config.Duration(10 * time.Second),
-		Log:     &testutil.Logger{Name: "huebridge"},
 	}

-	// Verify successful Init
-	require.NoError(t, h.Init())
+	for _, tt := range tests {
+		t.Run(tt.addr, func(t *testing.T) {
+			// The following URLs are all invalid must all be ignored during Init
+			plugin := &HueBridge{
+				BridgeUrls: []string{tt.addr},
+				Timeout:    config.Duration(10 * time.Second),
+				Log:        &testutil.Logger{Name: "huebridge"},
+			}

-	// Verify no bridge have been configured
-	require.Len(t, h.bridges, 0)
+			// Verify successful Init
+			require.ErrorContains(t, plugin.Init(), tt.expected)
+
+			// Verify no bridge have been configured
+			require.Empty(t, plugin.bridges)
+		})
+	}
 }

 func TestGatherLocal(t *testing.T) {
+	// Load the expected metrics
+	parser := &influx.Parser{}
+	require.NoError(t, parser.Init())
+	fn := filepath.Join("testdata", "metrics", "huebridge.txt")
+	expected, err := testutil.ParseMetricsFromFile(fn, parser)
+	require.NoError(t, err)
+	for i := range expected {
+		expected[i].SetType(telegraf.Gauge)
+	}
+
 	// Start mock server and make plugin targing it
 	bridgeMock := mock.Start()
 	require.NotNil(t, bridgeMock)
 	defer bridgeMock.Shutdown()
+
+	// Setup the plugin
 	h := &HueBridge{
 		BridgeUrls: []string{
 			fmt.Sprintf("address://%s:%s@%s/", mock.MockBridgeId, mock.MockBridgeUsername, bridgeMock.Server().Host),
@ -101,26 +141,10 @@ func TestGatherLocal(t *testing.T) {
 		Timeout:         config.Duration(10 * time.Second),
 		Log:             &testutil.Logger{Name: "huebridge"},
 	}
-
-	// Verify successful Init
 	require.NoError(t, h.Init())

-	// Verify successfull Gather
-	acc := &testutil.Accumulator{}
+	// Verify successfull collection
+	var acc testutil.Accumulator
 	require.NoError(t, acc.GatherError(h.Gather))
-
-	// Verify collected metrics are as expected
-	expectedMetrics := loadExpectedMetrics(t, "testdata/metrics/huebridge.txt", telegraf.Gauge)
-	testutil.RequireMetricsEqual(t, expectedMetrics, acc.GetTelegrafMetrics(), testutil.IgnoreTime(), testutil.SortMetrics())
-}
-
-func loadExpectedMetrics(t *testing.T, file string, vt telegraf.ValueType) []telegraf.Metric {
-	parser := &influx.Parser{}
-	require.NoError(t, parser.Init())
-	expectedMetrics, err := testutil.ParseMetricsFromFile(file, parser)
-	require.NoError(t, err)
-	for index := range expectedMetrics {
-		expectedMetrics[index].SetType(vt)
-	}
-	return expectedMetrics
+	testutil.RequireMetricsEqual(t, expected, acc.GetTelegrafMetrics(), testutil.IgnoreTime(), testutil.SortMetrics())
 }