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

Add support for stringlabels in Thanos Query #7645

Merged
merged 4 commits into from
Aug 18, 2024

Conversation

fpetkovski
Copy link
Contributor

@fpetkovski fpetkovski commented Aug 16, 2024

This commit finalizes support for the stringlabels build tag so that we can build the binary.

I would assume that we will still get panics if we run a store since the Series call still relies on casting one pointer type to another. This will be fixed in a follow up PR.

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Verification

This commit finalizes support for the stringlabels build tag
so that we can build the binary.

I would assume that we will still get panics if we run a store
since the Series call still relies on casting one pointer type
to another. This will be fixed in a follow up PR.

Signed-off-by: Filip Petkovski <[email protected]>
}
val, err := strconv.Unquote(parts[1])
if err != nil {
return nil, errors.Wrap(err, "unquote label value")
return labels.Labels{}, errors.Wrap(err, "unquote label value")
Copy link
Contributor

Choose a reason for hiding this comment

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

labels.EmptyLabels() maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had that first, but thought the empty struct is more idiomatic? I don't have a strong preference though

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO labels.EmtpyLabels read better as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SG, updated

MichaHoffmann
MichaHoffmann previously approved these changes Aug 16, 2024
Copy link
Contributor

@MichaHoffmann MichaHoffmann left a comment

Choose a reason for hiding this comment

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

lgtm, tahnk you

Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
}
// Compatibility label for Queriers pre 0.8.1. Filter it out now.
if ls[0].Name == store.CompatibilityTypeLabelName {
if ls.Len() == 0 {
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 removes the ancient store.CompatibilityTypeLabelName which I think is okay given how old the supported version is.

Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Would it be possible to add a build with stringlabels to CI?

@fpetkovski
Copy link
Contributor Author

fpetkovski commented Aug 16, 2024

The CI job should be there now. I wasn't sure how to add the build tag to promu so I just used go build.

Signed-off-by: Filip Petkovski <[email protected]>
Copy link
Member

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

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

LGTM!

${{ runner.os }}-go-

- name: Cross build check
run: go build -tags=stringlabels ./cmd/thanos
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's maybe keep it simple for now and not introduce another build tool

@fpetkovski fpetkovski merged commit e62dbeb into thanos-io:main Aug 18, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants