-
Notifications
You must be signed in to change notification settings - Fork 637
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
OG Go code cleanup #2017
OG Go code cleanup #2017
Conversation
@@ -343,7 +344,6 @@ github.com/chzyer/readline v1.5.1/go.mod h1:Eh+b79XXUwfKfcPLepksvw2tcLE/Ct21YObk | |||
github.com/chzyer/test v0.0.0-20180213035817-a1ea475d72b1 h1:q763qf9huN11kDQavWsoZXJNW3xEE4JJyHa5Q25/sd8= | |||
github.com/chzyer/test v1.0.0 h1:p3BQDXSxOhOG0P9z6/hGnII4LGiEPOYBhs8asl/fC04= | |||
github.com/chzyer/test v1.0.0/go.mod h1:2JlltgoNkt4TW/z9V/IzDdFaMTM2JPIi26O1pF38GC8= | |||
github.com/cilium/ebpf v0.11.0/go.mod h1:WE7CZAnqOL2RouJ4f1uyNhqr2P4CCvXFIqdRDUgWsVs= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why is it deleted? o_O
I'd expect it to be still used https://github.com/grafana/pyroscope/blob/next/ebpf/go.mod#L7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally not too familiar with workspaces, but according to stackoverflow go.work.sum
.
It would only contain things not contained/different from the referenced modules in go.work
.
As epbf/go.sum
contains this particular sum it can be removed. I also think our PR ci doesn't do a go mod tidy
check yet on ebpf
. That would also explain why it has been added in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should prevent it going forward: #2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Thanks for your work here, I am all for removing as much as we can. Once something is still required it can always make a comeback.
There are still a few linting issues to sort out. (I disabled linting of og/
before)
Part of #2010
This PR:
og/pkg
topkg/og
pyroscope-io/pyroscope
dependency