Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update go1.21 and CI #315

Merged
merged 2 commits into from Apr 9, 2024
Merged

Update go1.21 and CI #315

merged 2 commits into from Apr 9, 2024

Conversation

helsaawy
Copy link
Contributor

@helsaawy helsaawy commented Apr 1, 2024

Update go.mod to go1.21 to match hcsshim.

Use atomic.Bool stdlib instead of including our own.

Include tools\mkwinsyscall updates from #283 to switch to syscallN.
Note: removed // TODO about print/ln, since the latter adds spaces between args when printing, which is undesired.

Update testing code to fix unchecked-type-assertion lint.

Remove deprecated fields
Update CI to:

  • run build and lint steps on windows-2022 instead of windows-2019, similar to our hcsshim CI
  • use latest version of golangci-lint
    • remove deprecated fields in .golangci.yml
  • skip race detector on windows 2019 due to gcc/mingw-related errors

@helsaawy helsaawy requested a review from a team as a code owner April 1, 2024 18:59
Use `atomic.Bool` stdlib instead of including our own.

Include `tools\mkwinsyscall` updates from go-winio/283 to switch to
`syscallN`.
Note: removed `// TODO` about `print`/`ln`, since the latter adds spaces
between args when printing, which is undesired.

Also update CI to run steps on windows-2022 instead of windows-2019,
similar to our hcsshim CI.

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
hvsock_test.go Outdated
Comment on lines 133 to 136
sra, ok := (sv.RemoteAddr()).(*HvsockAddr)
if !ok {
t.Fatalf("expected type %T; got %T", new(HvsockAddr), sv.RemoteAddr())
}
Copy link
Member

Choose a reason for hiding this comment

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

You can make this a lot cleaner by defining a function:

func MustBeType[T any](t *testing.T, v any) T {
	v2, ok := v.(T)
	if !ok {
		t.Fatalf("expected type %T; got %T", *new(T), v)
	}
	return v2
}

and using it like:

sra := MustBeType[*HvsockAddr](t, sv.RemoteAddr())

@kevpar
Copy link
Member

kevpar commented Apr 8, 2024

This PR feels like it has a several unrelated changes, unless there's some interdepenency between them I'm not aware of. If not, it would be best to do these things as multiple PRs in the future. It's easier to review, and easier if we need to track down a specific change later on.

For now though, I'm okay getting this in (pending the other comment I left).

@msscotb
Copy link
Contributor

msscotb commented Apr 8, 2024

Looks ok but please do split unrelated changes in the future.

@helsaawy
Copy link
Contributor Author

helsaawy commented Apr 9, 2024

This PR feels like it has a several unrelated changes, unless there's some interdepenency between them I'm not aware of. If not, it would be best to do these things as multiple PRs in the future. It's easier to review, and easier if we need to track down a specific change later on.

For now though, I'm okay getting this in (pending the other comment I left).

The switch to go1.21 caused lint errors for the syscall.N and the race detector/gcc failure (and some weirdness with golangci-lint v1.53, iirc)
Upgrading golangci-lint introduced warning about the deprecated .golangci.yml fields and the unchecked-type-assertion linter (added in v1.55, via revive v1.3.4)

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

LGTM

@helsaawy helsaawy merged commit 3c9576c into microsoft:main Apr 9, 2024
8 checks passed
@helsaawy helsaawy deleted the updateGo branch April 9, 2024 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants