Restructured the whole code #4

DsgnrFH · 2025-10-31T14:06:34Z

Moved the netgear related things to a netgear package
Resolves #3

netgear package

cla-bot · 2025-10-31T14:06:37Z

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @DsgnrFH

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.
If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

DsgnrFH · 2025-10-31T14:11:46Z

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @DsgnrFH
* If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please [contact us](https://icinga.com/company/contact/) if you think this is the case.

* If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

test1

DsgnrFH · 2025-10-31T14:16:04Z

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @DsgnrFH

with a comment

bobapple · 2025-10-31T14:33:47Z

@cla-bot check

oxzi

First, thanks for creating this PR. I have skimmed over the code and made some first comments.

oxzi · 2025-10-31T15:29:03Z

netgear/netgear.go

+func NewNetgear(hostName, username, password string) *Netgear {
+	return &Netgear{
+		client:   &http.Client{Timeout: timeout},
+		hostName: strings.TrimSuffix(hostName, "/") + "/api/v1",


Consider using URL.JoinPath when constructing URLs.

Furthermore, Netgear.hostName is used as an URL throughout the code, thus consider changing its type to url.URL. And while doing so, consider renaming the variable since it carries an URL and not only a host name.

oxzi · 2025-10-31T15:32:12Z

netgear/netgear.go

+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return err
+	}
+
+	var result map[string]any
+	if err := json.Unmarshal(body, &result); err != nil {
+		return fmt.Errorf("failed to parse login response JSON: %w", err)
+	}


Instead of reading the whole body (of unknown size!) into a buffer, just to read the whole content afterwards, consider using the response body as an io.Reader. You can feed the .Body directly into json.NewDecoder.

oxzi · 2025-10-31T15:33:28Z

netgear/netgear.go

+
+	userRaw, ok := result["user"]
+	if !ok {
+		return fmt.Errorf("login response: missing 'user' field; body: %s", string(body))


I would advise against including the actually response body into an error message. For one, you cannot know how big the body is - potentially ending up with a huuuuge error. Furthermore, it may contain sensitive information.

oxzi · 2025-10-31T15:35:03Z

netgear/netgear.go

+
+	req, err := http.NewRequest(
+		http.MethodPost,
+		fmt.Sprintf("%s/login", n.hostName),


By changing Netgear.hostName to an url.URL as suggested above, you can just use .JoinPath for these URL constructions.

oxzi · 2025-10-31T15:39:00Z

netgear/netgear.go

+		return err
+	}
+	defer func() { _ = resp.Body.Close() }()
+	_, _ = io.ReadAll(resp.Body) // body ignored intentionally


Even if ignoring the return value, this would allocate a buffer and fill it once, just to be collected by the garbage collection later on. Instead of reading data into a buffer, consider streaming it into the void, io.Discard.

Suggested change

_, _ = io.ReadAll(resp.Body) // body ignored intentionally

_, _ = io.Copy(io.Discard, resp.Body)

oxzi · 2025-10-31T15:53:47Z

main.go

-	if err != nil { // Error is present, display it
+	n := netgear.NewNetgear(*hostName, *username, *password)
+	if err := n.Login(); err != nil {
 		fmt.Printf("Error while trying to login: %v\n", err)


When the check plugin fails to login, it just exits with exit code 0 - thus, OK. This should be an UNKNOWN.

oxzi · 2025-10-31T15:56:37Z

main.go

+		var deviceInfo netgear.DeviceInfo
+		inputData, err := n.DeviceInfo()
 		if err != nil {
-			panic(err)
+			fmt.Printf("Error retrieving device info: %v\n", err)
+			os.Exit(int(check.Unknown))
+		}
+		if err := json.Unmarshal(inputData, &deviceInfo); err != nil {
+			fmt.Printf("Failed to parse JSON: %v\n", err)
+			os.Exit(int(check.Unknown))
 		}


As I mentioned in the library, the caller should not do the conversion from some magic byte slice to a type, but this should happen in the library. Afterwards, this code would look way cleaner.

oxzi · 2025-10-31T15:59:38Z

main.go


-		// CPU check
+		// CPU
 		if !*hidecpu {


All these checks - CPU, Memory, Temperature, and so on - could be extracted into their own function, which is then just called here. This would help making the main function more readable. Furthermore, this might show repeated patterns for a further refactoring.

oxzi · 2025-10-31T16:01:13Z

main.go

+	CPU_WARN := pflag.Float64("cpu-warning", 50, "CPU usage warning threshold")
+	CPU_CRIT := pflag.Float64("cpu-critical", 90, "CPU usage critical threshold")
+	MEM_WARN := pflag.Float64("mem-warning", 50, "RAM usage warning threshold")
+	MEM_CRIT := pflag.Float64("mem-critical", 90, "RAM usage critical threshold")
+	FAN_WARN := pflag.Float64("fan-warning", 3000, "Fan speed warning threshold")
+	TEMP_WARN := pflag.Float64("temp-warning", 50, "Temperature warning threshold")
+	TEMP_CRIT := pflag.Float64("temp-critical", 70, "Temperature critical threshold")
+	STATS_WARN := pflag.Float64("stats-warning", 5, "Port stats warning threshold")
+	STATS_CRIT := pflag.Float64("stats-critical", 20, "Port stats critical threshold")


Usually, variables in Go are not written in ALL CAPS nor in snake_case. Something like flagCpuWarn, flagCpuCrit, … would be more idiomatic.

oxzi · 2025-10-31T16:02:28Z

main.go

+	}
+	return a
+}
+


There is a generic max function in the Go builtins.

… & remove custom maxStatus

Restructured the whole code, moved the netgear related things to a

1646aab

netgear package

DsgnrFH requested review from oxzi and yhabteab October 31, 2025 14:06

DsgnrFH self-assigned this Oct 31, 2025

DsgnrFH added the enhancement New feature or request label Oct 31, 2025

cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Oct 31, 2025

oxzi reviewed Oct 31, 2025

View reviewed changes

Modularise checks/utils, refactor netgear URL handling, improve flags…

5966aea

… & remove custom maxStatus

DsgnrFH requested a review from oxzi November 3, 2025 09:25

Fixed a few typos and addressed some comments

42c99a8

	_, _ = io.ReadAll(resp.Body) // body ignored intentionally
	_, _ = io.Copy(io.Discard, resp.Body)

Restructured the whole code #4

Are you sure you want to change the base?

Restructured the whole code #4

Conversation

DsgnrFH commented Oct 31, 2025

Uh oh!

cla-bot bot commented Oct 31, 2025

Uh oh!

DsgnrFH commented Oct 31, 2025

Uh oh!

DsgnrFH commented Oct 31, 2025

Uh oh!

bobapple commented Oct 31, 2025

Uh oh!

oxzi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants