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.

Sign up for GitHub

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

Jump to bottom

Update dskit version #5392

Merged
merged 14 commits into from
Feb 16, 2022
Merged

Update dskit version #5392

merged 14 commits into from
Feb 16, 2022

Conversation

MichelHollands
Copy link
Contributor

@MichelHollands MichelHollands commented Feb 15, 2022 •
edited
Loading

What this PR does / why we need it:

Update the dskit version and update the Loki code so the new extra logger parameter for the RegisterFlags function of the LifecyclerConfig is given when registering the flags of the ingester.

This latest dskit version also updates the LifecyclerConfig so that only private interfaces are used. This required changes to unit tests. Additionally the private interface call was added to other rings: frontend, ruler and distributor. These do not use the LifeCycler from dskit.

The CircleCI config was updated as well because dskit now requires Go 1.17.

Which issue(s) this PR fixes:
N/A

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated
  • Add an entry in the CHANGELOG.md about the changes.

Signed-off-by: Michel Hollands [email protected]

@MichelHollands
Update dskit version
e234720 
Signed-off-by: Michel Hollands <[email protected]>
@MichelHollands MichelHollands requested a review from a team as a code owner February 15, 2022 09:55
@MichelHollands
update changelog
d83c9c6 
Signed-off-by: Michel Hollands <[email protected]>
@dannykopping
Copy link
Contributor

with the 1 impact

Could you elaborate on this please @MichelHollands?

@MichelHollands
Copy link
Contributor Author

with the 1 impact

Could you elaborate on this please @MichelHollands?

The PR description has been updated so it mentions the extra parameter required by dskit in the ingester code.

dannykopping
dannykopping reviewed Feb 15, 2022
View reviewed changes
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM, ping me once you've got the tests passing and I'll approve + merge

@MichelHollands MichelHollands marked this pull request as draft February 15, 2022 10:19
@pull-request-size pull-request-size bot added size/M and removed size/S labels Feb 15, 2022
@MichelHollands MichelHollands marked this pull request as ready for review February 15, 2022 15:46
DylanGuedes
DylanGuedes approved these changes Feb 15, 2022
View reviewed changes
Copy link
Contributor

@DylanGuedes DylanGuedes left a comment

Choose a reason for hiding this comment

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

LGTM! just a few minor questions

CHANGELOG.md Outdated Show resolved Hide resolved
vendor/github.com/grafana/dskit/kv/etcd/etcd.go Show resolved Hide resolved
vendor/github.com/grafana/dskit/ring/lifecycler.go Show resolved Hide resolved
dannykopping
dannykopping approved these changes Feb 16, 2022
View reviewed changes
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

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

LGTM, I'm interested in your answers to @DylanGuedes' comments though before we merge

simonswine
simonswine reviewed Feb 16, 2022
View reviewed changes
Copy link
Contributor

@simonswine simonswine left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for pulling in this dskit improvement.

I think it would also make sense to override the doc: annotation:

InstanceInterfaceNames []string `yaml:"instance_interface_names" doc:"default=[<private network interfaces>]"

I have also noticed that you have not touched the InstanceInterfaceNames in pkg/loki/common/common.go

@MichelHollands
Copy link
Contributor Author

MichelHollands commented Feb 16, 2022 •
edited by simonswine
Loading

Thanks for pulling in this dskit improvement.

I think it would also make sense to override the doc: annotation:

InstanceInterfaceNames []string `yaml:"instance_interface_names" doc:"default=[<private network interfaces>]"

I have also noticed that you have not touched the InstanceInterfaceNames in pkg/loki/common/common.go

@simonswine
The doc annotation has been added. The documentation has been updated as well.
Good catch with common.go. That has been updated now as well.

simonswine
simonswine approved these changes Feb 16, 2022
View reviewed changes
Copy link
Contributor

@simonswine simonswine left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments. I think it's ready to go 🚀 . LGTM

@dannykopping dannykopping merged commit d8ed87d into grafana:main Feb 16, 2022
@dannykopping dannykopping mentioned this pull request Feb 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DylanGuedes DylanGuedes DylanGuedes approved these changes

@dannykopping dannykopping dannykopping approved these changes

@simonswine simonswine simonswine approved these changes

@KMiller-Grafana KMiller-Grafana Awaiting requested review from KMiller-Grafana

Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MichelHollands @dannykopping @simonswine @DylanGuedes