Tooling updates + add Proxy support to httpclient.DefaultTransport #414

mmlb · 2025-03-11T18:09:46Z

What does this PR implement/change/remove?

Some spring cleaning of non-source files
Addition of Proxy config to the httpclient.DefaultTransport
Add example binaries to .gitignore

Spring cleaning changes are annoyingly large but mostly mechanical, either via gofumpt or golangci-lint run --fix, there were some manual fixes I made to avoid disabling linters that complained about a handful of things.

I also added Proxy config to the DefaultTransport just like the DefaultTransport in the http package does. This setup will make it so http requests will go via a proxy when HTTPS_PROXY env var exists. This way we can debug redfish over https using mitmproxy or similar setup.

I added the binaries to .gitignore because they should have been there from the beginning.

Description for changelog/release notes

https connections now go through a proxy if the HTTPS_PROXY environment variable is set

jacobweinstock · 2025-03-11T18:55:28Z

go.mod

@@ -1,6 +1,6 @@
 module github.com/bmc-toolbox/bmclib/v2

-go 1.21
+go 1.23


I recommend that we don't bump this version to 1.23. This version doesn't correspond to Go's supported versions but "The minimum version of Go required to compile packages in this module." - ref.

The current bmclib policy is, "As a library we will only bump the version of Go in the go.mod file when there are required dependencies in bmclib that necessitate a version bump." - ref

From this PR, the only thing I can tell that needs the version to increment is for x := range 5. I believe this was introduced in Go 1.22. So the conversation should be around whether to go from 1.21 to 1.22 and not 1.21 to 1.23. I am onboard with going to 1.22 but not 1.23.

Right, but no one should be running go <1.23 anyway. I kind of see it as a chicken/egg situation. When 1.25 comes out I'd like to bump this to 1.24 and then make use of go tool support to run various tools (like golangci-lint) via native go pinning.

Sorry, that's not the policy. As a library, we shouldn't dictate the version clients use. Per the policy, go tool support isn't a reason to update this. There are many examples of older Go versions in large active Go module repos in the wild. https://github.com/spf13/cobra, for example.

oh til! well will back this out then

there's also the copyloopvar change that came in from 1.22 that affects this repo (albeit just one line). I'll back this out and have a separate PR for 1.22 + enabling those linters.

Alright dropped back down to 1.21 and undid changes

jacobweinstock · 2025-03-11T18:59:45Z

providers/asrockrack/helpers.go

@@ -602,7 +600,7 @@ func (a *ASRockRack) queryHTTPS(ctx context.Context, endpoint, method string, pa
 	}

 	// add headers
-	req.Header.Add("X-CSRFTOKEN", a.loginSession.CSRFToken)
+	req.Header.Add("X-Csrftoken", a.loginSession.CSRFToken)


What's motivating this change? I believe there was a code comment elsewhere that referenced old firmware needing some specific casing. Regardless, the HTTP spec says that headers are case-insensitive. ref

this was an autofix from one of the linters, can do the magic comment to have it ignore this but I'd like some clarity before going that route.

I'm not seeing anything in the code base about old firmware expecting specific case of headers, are you thinking about supermicro with form-data fields https://github.com/bmc-toolbox/bmclib/blob/main/providers/supermicro/floppy.go#L91-L96?

I'd recommend the headers are left as is, unless a change is required in practice

should be back to original format

jacobweinstock · 2025-03-11T19:04:19Z

internal/redfishwrapper/task.go

@@ -76,13 +74,13 @@ func (c *Client) ConvertTaskState(state string) constants.TaskState {
 	switch strings.ToLower(state) {
 	case "starting", "downloading", "downloaded", "scheduling":
 		return constants.Initializing
-	case "running", "stopping", "cancelling":
+	case "running", "stopping", "canceling":


I don't think these should change as they are States that correspond to the gofish library. https://github.com/stmcginnis/gofish/blob/4b51c15f0a3df649821f9fc395a82483ff60bb2a/redfish/task.go#L16-L55

It is spelled Cancelling in that library. We should probably be using the const from that package anyway.

uggh good catch will back out. We are using the consts from that package already. The problem is we are converting that package's types <-> string when it should be done by that package itself (via stringer imo).

should be back to original (plus //nolint: directive)

jacobweinstock · 2025-03-11T19:09:29Z

client.go

+	client := *old
+	transport, ok := client.Transport.(*http.Transport)
+	if !ok {
+		panic("old client's transport is not expected type")


hmm...I'm concerned about this panic. I know we weren't even checking the type assert before, and it could have implicitly caused a panic already, but could we introduce a more graceful way to handle the bad type assert? Maybe a return error or something similar?

I don't think its worth it, changing to an error return is going to have wide effect when done for registerASRRProvider and probably others (bubbling the error up). Its never really going to be a problem either since we aren't really using different implementations. If it ever changes then it'll be caught early at startup instead of later at runtime.

Not a whole lot of benefit imo.

This does seem like an exceptional case, can the panic message include a clearer message, along the lines of "expected a http client of the http.Transport type, got $blah instead"?

I've updated the panic message

joelrebel

Thanks for updates and fixes @mmlb , for the reviewers sake, going ahead could we keep the PRs scope limited

joelrebel · 2025-03-12T09:42:12Z

client.go

+	client := *old
+	transport, ok := client.Transport.(*http.Transport)
+	if !ok {
+		panic("old client's transport is not expected type")


This does seem like an exceptional case, can the panic message include a clearer message, along the lines of "expected a http client of the http.Transport type, got $blah instead"?

Automated using the following command: ``` git ls-files '*.go' | xargs -I% sh -c ' sed "/^import (/,/^)/ { /^\s*$/ d }" % >%.tmp && goimports -w %.tmp && (if cmp -s % %.tmp; then rm %.tmp; else mv %.tmp %; fi)' ```

mmlb · 2025-03-12T15:32:06Z

Thanks for updates and fixes @mmlb , for the reviewers sake, going ahead could we keep the PRs scope limited

I'm going to drop new features from this PR and just do the format/lint fixes, aka the cleaning part

Going to go from disable-all,enable-some to enable-all,disable-some so we are going to want to avoid golangci-lint changing out from under us.

Drop enabling govets shadow because it's too annoying

mmlb · 2025-03-12T19:41:39Z

Thanks for updates and fixes @mmlb , for the reviewers sake, going ahead could we keep the PRs scope limited

I've trimmed things down and also split the commits a little bit so that they are easier to review. I split out what was previously "automatic fixes + manual fixes" into multiple ones. The first iteration was all together so that each commit would still pass linting so it was quite large. Now a couple of intermediate commits won't pass but makes for smaller diffs.

joelrebel

This looks fine to me

jacobweinstock

Thanks for updates and fixes @mmlb , for the reviewers sake, going ahead could we keep the PRs scope limited

I don't think multiple commits was what @joelrebel was driving at here. Limiting scope would be, for example, creating a PR just for the linting changes, one for the small pedantic differences you prefer, and another for just the proxy support.

jacobweinstock · 2025-03-13T15:26:21Z

providers/supermicro/firmware_bios_test.go

@@ -93,7 +94,7 @@ func Test_setComponentUpdateMisc(t *testing.T) {
 					return
 				}

-				assert.Nil(t, err)
+				assert.NoError(t, err)


The use of require.NoError and assert.NoError seems arbitrary. When to use which is not clear. Is this something that golangci-lint catches? If not, I recommend either creating a doc for when one should be used over the other, or just stick to one.

jacobweinstock · 2025-03-13T15:28:24Z

internal/redfishwrapper/task.go

+	case constants.PowerCycleHost, constants.Unknown:
+		fallthrough


For what is this optimizing? Why is this needed if it's just going to fallthrough to the default? Won't the default case be called regardless of whether this case exists or not?

Oh, is this to satisfy a linting complaint? If so, I recommend a code comment to improve understandability and the required cognitive load.

constants.PowerCycleHost and constants.Unknown should not be part of the switch case since they are not Redfish task states,
could we have this case removed?

http://redfish.dmtf.org/schemas/DSP2046_2019.4.html#redfish.dmtf.org/schemas/v1/Task.json|details|TaskState

mmlb requested a review from joelrebel March 11, 2025 18:09

mmlb added the ready-to-merge label Mar 11, 2025

mmlb force-pushed the push-muqtvtlsqyrn branch 3 times, most recently from 6e6567e to 8d1a4b3 Compare March 11, 2025 18:51

jacobweinstock requested changes Mar 11, 2025

View reviewed changes

joelrebel requested changes Mar 12, 2025

View reviewed changes

mmlb added 3 commits March 12, 2025 11:24

examples: Ignore binaries

886d623

Reformat with gofumpt

6d8b23c

Get rid of unnecessary groups in imports

bace014

Automated using the following command: ``` git ls-files '*.go' | xargs -I% sh -c ' sed "/^import (/,/^)/ { /^\s*$/ d }" % >%.tmp && goimports -w %.tmp && (if cmp -s % %.tmp; then rm %.tmp; else mv %.tmp %; fi)' ```

mmlb added 3 commits March 12, 2025 12:02

Pin golangci-lint version in github action

9b560cb

Going to go from disable-all,enable-some to enable-all,disable-some so we are going to want to avoid golangci-lint changing out from under us.

Update .golangci.yml to version based on file from metal-toolbox org

2347ae2

Apply automatic fixes from golangci-lint linters

e7ee285

mmlb force-pushed the push-muqtvtlsqyrn branch from 8d1a4b3 to 886d623 Compare March 12, 2025 19:00

Apply manual fixes to appease linters

eb44621

Drop enabling govets shadow because it's too annoying

mmlb force-pushed the push-muqtvtlsqyrn branch from 39f3295 to 95d4d4c Compare March 12, 2025 19:31

Undisable more golangci-lint linters

0d3b939

mmlb force-pushed the push-muqtvtlsqyrn branch from 95d4d4c to 0d3b939 Compare March 12, 2025 19:38

mmlb requested review from jacobweinstock and joelrebel March 13, 2025 00:01

joelrebel approved these changes Mar 13, 2025

View reviewed changes

jacobweinstock reviewed Mar 13, 2025

View reviewed changes

joelrebel removed the ready-to-merge label Mar 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tooling updates + add Proxy support to httpclient.DefaultTransport #414

Tooling updates + add Proxy support to httpclient.DefaultTransport #414

mmlb commented Mar 11, 2025 •

edited

Loading

jacobweinstock Mar 11, 2025

mmlb Mar 11, 2025

jacobweinstock Mar 11, 2025

mmlb Mar 11, 2025

mmlb Mar 12, 2025

mmlb Mar 12, 2025

jacobweinstock Mar 11, 2025

mmlb Mar 11, 2025

joelrebel Mar 12, 2025

mmlb Mar 12, 2025

mmlb Mar 12, 2025

jacobweinstock Mar 11, 2025

mmlb Mar 11, 2025

mmlb Mar 12, 2025

jacobweinstock Mar 11, 2025

mmlb Mar 11, 2025

joelrebel Mar 12, 2025

mmlb Mar 12, 2025

joelrebel left a comment

joelrebel Mar 12, 2025

mmlb commented Mar 12, 2025

mmlb commented Mar 12, 2025

joelrebel left a comment

jacobweinstock left a comment

jacobweinstock Mar 13, 2025

jacobweinstock Mar 13, 2025

jacobweinstock Mar 13, 2025 •

edited

Loading

joelrebel Mar 13, 2025

		case constants.PowerCycleHost, constants.Unknown:
		fallthrough

Tooling updates + add Proxy support to httpclient.DefaultTransport #414

Are you sure you want to change the base?

Tooling updates + add Proxy support to httpclient.DefaultTransport #414

Conversation

mmlb commented Mar 11, 2025 • edited Loading

What does this PR implement/change/remove?

Description for changelog/release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelrebel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmlb commented Mar 12, 2025

mmlb commented Mar 12, 2025

joelrebel left a comment

Choose a reason for hiding this comment

jacobweinstock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobweinstock Mar 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmlb commented Mar 11, 2025 •

edited

Loading

jacobweinstock Mar 13, 2025 •

edited

Loading