Skip to content

restore full go vet behaviour #28945

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

Merged
merged 5 commits into from
Nov 6, 2023

Conversation

bimmlerd
Copy link
Member

@bimmlerd bimmlerd commented Nov 2, 2023

This PR contains three commits to fix up go vet ./... which had regressed, and a fourth commit to reinstate running it as part of CI.

This effectively reverts #26978, which seems fine as the PR which is linked as requiring it was never merged. Additionally, the work which seems to supersede it doesn't appear to require the deepcopy stuff #28873 (feel free to correct me here, @marqc).

cc @tklauser for the original change to drop go vet.

@maintainer-s-little-helper

This comment was marked as outdated.

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Nov 2, 2023
This reverts commit cc94abe.

Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
This reverts commit 9f4dcea.

Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
@bimmlerd bimmlerd force-pushed the pr/bimmlerd/restore-full-go-vet branch from 9de604a to cfe4784 Compare November 2, 2023 13:12
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Nov 2, 2023
@bimmlerd bimmlerd force-pushed the pr/bimmlerd/restore-full-go-vet branch from cfe4784 to 9d72400 Compare November 2, 2023 13:30
@bimmlerd bimmlerd had a problem deploying to release-base-images November 2, 2023 13:30 — with GitHub Actions Failure
@bimmlerd bimmlerd added area/CI Continuous Integration testing issue or flake release-note/ci This PR makes changes to the CI. sig/hubble sig/community Impacts contribution workflow, guidelines, and tools. labels Nov 2, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 2, 2023
Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
Avoid copying the flow protobuf message, as this triggers go vet to
complain about copying a mutex (in internal protobuf state). No
functional changes intended, simply appeasing vet.

Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
Without -vet=all, go test doesn't run the same set of vets as go vet.
Since we dropped go vet ./... from the integration tests, we weren't
running the full suite anymore.

Fixes: f6346d2 (make: drop redundant `go vet ./...` from integration tests)

Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
@bimmlerd bimmlerd force-pushed the pr/bimmlerd/restore-full-go-vet branch from 9d72400 to ca14e6d Compare November 2, 2023 13:55
@bimmlerd bimmlerd temporarily deployed to release-base-images November 2, 2023 13:55 — with GitHub Actions Inactive
@bimmlerd bimmlerd marked this pull request as ready for review November 2, 2023 14:09
@bimmlerd bimmlerd requested review from a team as code owners November 2, 2023 14:09
@bimmlerd bimmlerd requested review from tklauser and kaworu November 2, 2023 14:09
@bimmlerd
Copy link
Member Author

bimmlerd commented Nov 2, 2023

/test

Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Hubble changes LGTM, thanks @bimmlerd!

@marqc
Copy link
Contributor

marqc commented Nov 2, 2023

@bimmlerd LGTM
I believe the original change was made by @AwesomePatrol because the initial attempt for dynamic flowlogging was to use CRDs with FlowFilters. This is not required by configuration stored in yaml files.

@bimmlerd
Copy link
Member Author

bimmlerd commented Nov 2, 2023

Tests reliant on 1.1.1.1 are failing due to https://www.cloudflarestatus.com/incidents/hm7491k53ppg, I'll wait with retriggering until that is resolved.

@bimmlerd
Copy link
Member Author

bimmlerd commented Nov 6, 2023

/ci-ipsec-upgrade

@bimmlerd
Copy link
Member Author

bimmlerd commented Nov 6, 2023

/ci-awscni

@bimmlerd
Copy link
Member Author

bimmlerd commented Nov 6, 2023

/ci-clustermesh

@bimmlerd
Copy link
Member Author

bimmlerd commented Nov 6, 2023

/ci-e2e

@bimmlerd
Copy link
Member Author

bimmlerd commented Nov 6, 2023

/ci-eks

@bimmlerd
Copy link
Member Author

bimmlerd commented Nov 6, 2023

/ci-ipsec-e2e

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 6, 2023
@aditighag aditighag merged commit fb2f404 into cilium:main Nov 6, 2023
@bimmlerd bimmlerd deleted the pr/bimmlerd/restore-full-go-vet branch November 6, 2023 15:06
@tklauser tklauser added backport/1.14 This PR represents a backport for Cilium 1.14.x of a PR that was merged to main. needs-backport/1.14 and removed backport/1.14 This PR represents a backport for Cilium 1.14.x of a PR that was merged to main. needs-backport/1.14 labels Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI Continuous Integration testing issue or flake ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI. sig/community Impacts contribution workflow, guidelines, and tools.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants