Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

Refactor metrictank repo to be compatible with go modules #2042

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

ywwg
Copy link
Contributor

@ywwg ywwg commented Oct 7, 2022

This PR refactors the entire metrictank repository to make it compatible with Go modules as well with vscode's static compilation. All of the Makefile commands have been fixed (as far as I know). I believe travis/circleci is also working.

As part of the change, the vendor/ directory has been removed from checked-in source, instead users should run go mod vendor locally as needed.

@ywwg ywwg marked this pull request as ready for review October 7, 2022 16:30
@ywwg ywwg requested review from Dieterbe and replay October 7, 2022 17:40
@ywwg ywwg force-pushed the owilliams/gofixes2 branch 3 times, most recently from 1254c68 to 731321a Compare October 11, 2022 19:17
@@ -75,7 +75,7 @@ func main() {
fmt.Fprintln(os.Stderr, "Copies data in Cassandra to use another table (and possibly another cluster).")
fmt.Fprintln(os.Stderr, "It is up to you to assure table-out exists before running this tool")
fmt.Fprintln(os.Stderr, "This tool is EXPERIMENTAL and needs doublechecking whether data is successfully written to Cassandra")
fmt.Fprintln(os.Stderr, "see https://github.com/grafana/metrictank/pkg/pull/909 for details")
fmt.Fprintln(os.Stderr, "see https://github.com/grafana/metrictank/pull/909 for details")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was a find-replace error

// and singles in groups by retention (in schemaID order)
// * PNGroups obviously will need a common interval, which gets interesting when using multiple schemas
// * coarsening continues until all data is fetched at its coarsest. At that point we may breach soft, but never hard
// - requests are coarsened, PNGroup by PNGroup (we cannot predict PNGroup map iteration order, so we only test with 1 PNGroup),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the new gofmt does different things

pkg/api/graphite.go Outdated Show resolved Hide resolved
@@ -64,7 +64,7 @@ var (
// also takes a "now" value which we compare the TTL against

// Note: MDP-yes code path may not take into account that archive 0 may have a different raw interval.
// see https://github.com/grafana/metrictank/issues/1679
// see https://github.com/grafana/metrictank/pkg/issues/1679
Copy link
Contributor

Choose a reason for hiding this comment

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

same

pkg/api/response/error.go Outdated Show resolved Hide resolved
pkg/expr/plan_test.go Outdated Show resolved Hide resolved
pkg/mdata/aggmetrics.go Outdated Show resolved Hide resolved
pkg/schema/msg/format.go Outdated Show resolved Hide resolved
@replay
Copy link
Contributor

replay commented Oct 11, 2022

I think to fix the pointed out search & replace issue the easiest would be to just search and replace these strings:

github.com/grafana/metrictank/pkg/blob
github.com/grafana/metrictank/pkg/issue(s)
github.com/grafana/metrictank/pkg/pull(s)

and remove the pkg again from them

@ywwg
Copy link
Contributor Author

ywwg commented Oct 12, 2022

ok I rolled the deps back that I could get from gopkg.lock, but unfortunately there are still massive diffs between the checked-in vendor/ dir in metrictank and the versions created by go mod vendor. What's more, it looks like the checked-in vendor/ dir has been hacked up, removing README and CHANGES files, making it impossible to know which version was downloaded. And given the huge number for dependencies I'm not sure there's a reliable way to pin all the deps at the correct old versions.

I think the next step is figuring out a way to test this branch to see if any of the module updates break anything.

@ywwg
Copy link
Contributor Author

ywwg commented Oct 18, 2022

due to the risk of instability, we are going to put off merging this until ~Q1 or so.

@ywwg ywwg marked this pull request as draft October 18, 2022 15:31
@ywwg ywwg marked this pull request as ready for review May 25, 2023 14:39
@ywwg ywwg requested a review from carrieedwards May 25, 2023 14:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants