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

Update to latest dskit #3915

Merged
merged 6 commits into from
Jul 29, 2024
Merged

Conversation

andreasgerstmayr
Copy link
Contributor

What this PR does:
Update to latest dskit

Which issue(s) this PR fixes:
grafana/dskit#518
grafana/dskit#537
Prerequisite for #3646

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
@joe-elliott
Copy link
Member

Thanks for the PR andreas! Heads up tests are broken atm: #3916

@andreasgerstmayr
Copy link
Contributor Author

Thanks for the PR andreas! Heads up tests are broken atm: #3916

Ah, that explains stuff. Thanks for the info. I got confused why the latest commit on the main branch is green, but I just noticed now that the unit tests do not run on the main branch commits. I assumed it's maybe a mismatch of my local Go version.

In the meantime, I hacked together a small util which shuffles the query parameters around (no actual value change) and manually updated the test cases in search_sharder_test.go .

@joe-elliott
Copy link
Member

In the meantime, I hacked together a small util which shuffles the query parameters around (no actual value change) and manually updated the test cases in search_sharder_test.go .

Yeah, I considered this as well. As far as I can tell the order is deterministic (no map iteration or other normal things that break these kind of things). Are tests passing regularly?

@joe-elliott
Copy link
Member

@andreasgerstmayr got my PR merged and i think i prefer it to yours b/c it will survive future changes to the way we build those urls. can you drop the changes to the search sharder test?

@andreasgerstmayr
Copy link
Contributor Author

@andreasgerstmayr got my PR merged and i think i prefer it to yours b/c it will survive future changes to the way we build those urls. can you drop the changes to the search sharder test?

Good point! I dropped the search sharder test changes from this PR.

Copy link
Member

@joe-elliott joe-elliott left a comment

Choose a reason for hiding this comment

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

Thanks!

@joe-elliott joe-elliott merged commit 54c8e05 into grafana:main Jul 29, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants