-
Notifications
You must be signed in to change notification settings - Fork 66
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
Add min/max time filter to blockscopy and change min duration default to 0 #261
Conversation
… to 0 Signed-off-by: Marco Pracucci <marco@pracucci.com>
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.
Change seems reasonable to me. Should we have test coverage for this?
(The linting error appears unrelated).
We don't have tests for this tool. Ideally yes, but for many tools we don't have tests. |
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
cmd/blockscopy/main.go
Outdated
@@ -50,7 +52,9 @@ type config struct { | |||
func (c *config) RegisterFlags(f *flag.FlagSet) { | |||
f.StringVar(&c.sourceBucket, "source-bucket", "", "Source GCS bucket with blocks.") | |||
f.StringVar(&c.destBucket, "destination-bucket", "", "Destination GCS bucket with blocks.") | |||
f.DurationVar(&c.minBlockDuration, "min-block-duration", 24*time.Hour, "If non-zero, ignore blocks that cover block range smaller than this.") | |||
f.DurationVar(&c.minBlockDuration, "min-block-duration", 0, "If non-zero, ignore blocks that cover block range smaller than this.") | |||
f.Var(&c.minTime, "min-time", "If set, only blocks with MinTime >= this value are copied.") |
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.
Can you add an example of the expected format for these?
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.
Good idea. Done in: ce4619c
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Lint fails because of the old go version required. I'm going to merge cause I will move this tool to Mimir repo. |
Signed-off-by: Josh Carp <jm.carp@gmail.com>
I'm running a migration between two Grafana Mimir clusters and using the
blockscopy
tool to copy blocks. I've done some changes toblockscopy
to support me, which I would like to upstream in this PR: