Properly assign logrus entry for fallback queries

[dcantah]

I didn't reassign the logEntry that contained the error reason if
querying for stats in the shim failed

[kevpar]

LGTM

[dcantah]

Interesting new failure..:

=== RUN TestGetCommandPath_WithLookPath
runhcs_test.go:49: expected fake path 'D:\a\hcsshim\hcsshim\pkg\go-runhcs\runhcs.exe' got 'runhcs.exe'
--- FAIL: TestGetCommandPath_WithLookPath (0.03s)

It's picking go 1.19 on ws2022 and 1.18.4 on ws2019 which is interesting. Something might've changed in LookPath or atomic, or our test was just bad. I'll look

[kevpar]

Interesting new failure..:

=== RUN TestGetCommandPath_WithLookPath runhcs_test.go:49: expected fake path 'D:\a\hcsshim\hcsshim\pkg\go-runhcs\runhcs.exe' got 'runhcs.exe' --- FAIL: TestGetCommandPath_WithLookPath (0.03s)

It's picking go 1.19 on ws2022 and 1.18.4 on ws2019 which is interesting. Something might've changed in LookPath or atomic, or our test was just bad. I'll look

IIRC there was a path lookup change in go1.19. Why is the CI pulling in a new Go version that we haven't explicitly switched to yet, though?

[dcantah]

We specify ^1.17.0 for the version which should just ask for anything greater than or equal to 1.17.x but less than 2.0. Not sure why it only picked 1.19 for ws2022 however

hcsshim/.github/workflows/ci.yml

Line 113 in 2a9d2d9

go-version: "^1.17.0"

[kevpar]

We specify ^1.17.0 for the version which should just ask for anything greater than or equal to 1.17.x but less than 2.0. Not sure why it only picked 1.19 for ws2022 however

hcsshim/.github/workflows/ci.yml

Line 113 in 2a9d2d9

go-version: "^1.17.0"

That seems like a bug. I imagine this expression was intended to mean "latest patch release of 1.17".

[helsaawy]

That seems like a bug. I imagine this expression was intended to mean "latest patch release of 1.17".

Is it worthwhile to matrix go versions as well, since knowing that go1.19 causes test failures is good to catch early on ...

[dcantah]

setup-go apparently uses https://github.com/npm/node-semver to match, which has this as documented behavior. https://github.com/npm/node-semver#caret-ranges-123-025-004

^1.2.3 := >=1.2.3 <2.0.0-0

[kevpar]

setup-go apparently uses https://github.com/npm/node-semver to match, which has this as documented behavior. https://github.com/npm/node-semver#caret-ranges-123-025-004

^1.2.3 := >=1.2.3 <2.0.0-0

Looks like 1.17.* is what we would want.

[dcantah]

It's a bit odd to be able to get a new minor go version without specifying, but it is kind of nice how we found this due to that. Hamza's suggestion is valid, maybe we should test against multiple go versions? The alternative I guess being whenever we decide to get onboard 1.18/19/20 etc. and update our ci, we find the errors and can fix them in one sweep lol

[kevpar]

It's a bit odd to be able to get a new minor go version without specifying, but it is kind of nice how we found this due to that. Hamza's suggestion is valid, maybe we should test against multiple go versions? The alternative I guess being whenever we decide to get onboard 1.18/19/20 etc. and update our ci, we find the errors and can fix them in one sweep lol

I'm okay with doing a matrix if we can make it work, but I don't want to block fixing what seems like a very clear bug here in our version selection. Can we just make a small change here so we don't grab 1.19 before we intend to?

[dcantah]

We've known we were doing this (at least I did lol), but I can swap to 1.17.* here

[dcantah]

Should we get onto 1.18 seeming as how 1.17 is eol

[kevpar]

Should we get onto 1.18 seeming as how 1.17 is eol

Agreed we should move everything forward. Let's do that in a separate PR though.

[dcantah]

I was going to open a PR to update to 1.18.x in one go for our go-version usages and go.mod and just rebase this after.