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 changelog, fix acl-token flags in help. #1587

Merged
merged 3 commits into from
Apr 1, 2022
Merged

Conversation

pstibrany
Copy link
Member

What this PR does

This PR updates dskit to commit 6d48ca07ade5.. This fixes panic in multi KV store implementation when using runtime config to change primary KV store used by multi.

Checklist

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

Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

I checked the dskit diff and LGTM. However, there's some issue with -help which is now displaying an flag categorized as advanced. Could you look into it?

@@ -87,6 +87,8 @@ Usage of ./cmd/mimir/mimir:
Comma-separated list of network CIDRs to block in Alertmanager receiver integrations.
-alertmanager.receivers-firewall-block-private-addresses
True to block private and local addresses in Alertmanager receiver integrations. It blocks private addresses defined by RFC 1918 (IPv4 addresses) and RFC 4193 (IPv6 addresses), as well as loopback, local unicast and local multicast addresses.
-alertmanager.sharding-ring.consul.acl-token string
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's categorized as advanced. It shouldn't be here.

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to distinguish between outer struct and inner field. Right now categorization finds the inner field with same address as outer struct, and replaces the field information. If we make inner field unexported, we can ignore such field in Mimir's code generating flag. I've posted PR grafana/dskit#154 to accomplish that.

Copy link
Collaborator

@pracucci pracucci Mar 31, 2022

Choose a reason for hiding this comment

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

How cool is the fact that we have a preview of the -help in cmd/mimir/help.txt.tmpl? 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

😄

@pstibrany
Copy link
Member Author

Please rebase this PR on top of main to make all required test checks pass.

@@ -127,3 +127,9 @@ func (m *mockTenantConfigProvider) S3SSEKMSKeyID(_ string) string {
func (m *mockTenantConfigProvider) S3SSEKMSEncryptionContext(_ string) string {
return m.s3KmsEncryptionContext
}

func secretWithValue(v string) flagext.Secret {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would upstream this to dskit. Looks a very handy utility.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM!

@pstibrany
Copy link
Member Author

I think I will try another approach, simply have a list of structs that should not be descended into. It should result in cleaner code with less weird rules about exported/unexported fields, and smaller PR.

@pstibrany pstibrany mentioned this pull request Mar 31, 2022
3 tasks
Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
…if it's advanced field or not.

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
@pstibrany pstibrany changed the title Update dskit with bugfix to multikv. Update changelog, fix acl-token flags in help. Apr 1, 2022
@pstibrany
Copy link
Member Author

I've rebased this PR on top of main. Since dskit was already updated in #1599, in this PR I'm only updating CHANGELOG to mention multikv fix, and fix problem with acl-token arguments.

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@pstibrany pstibrany merged commit 9f874de into main Apr 1, 2022
@pstibrany pstibrany deleted the update-dskit branch April 1, 2022 08:46
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