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

mimirtool config: GEM conversions & ring.instance_id #1513

Merged
merged 6 commits into from
Mar 21, 2022

Conversation

dimitarvdimitrov
Copy link
Contributor

What this PR does

  • slightly refactors convert_test.go:
    • it's easier to reuse the logic for adding common flags (e.g. server.http_listen_port and from now also GEM's auth.type)
    • separates some shared test cases (server.http_listen_port)
  • updates mimir, cortex, and GEM descriptors:
    • adds hidden config options (e.g. -ingester.lifecycler.id and -distributor.ring.instance_id)
    • replaces the default value of instance_id in GEM and Mimir 2.0 with null - this is done manually until we can support the dynamic flags (ones that detect the default value automatically)
    • updates the GEM descriptor to one from master
  • adds support for the rest of the GEM flags - moved, renamed + memcached migrations, etc.

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

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

Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Looks good. < encoding as \u003c is fine, and "prefix" fields are coming from Cortex structs with doc:"hidden" tags. Missing ) in addresses field is also correct, and should be fixed in Cortex first.

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
@dimitarvdimitrov
Copy link
Contributor Author

@pstibrany I've moved the GEM-specific removed options to gem.go. As far as I understand the rest of the comments are no longer a concern?

@pstibrany
Copy link
Member

pstibrany commented Mar 21, 2022

@pstibrany I've moved the GEM-specific removed options to gem.go. As far as I understand the rest of the comments are no longer a concern?

I resolved comments that are not a concern anymore. One remaining comment is a non-blocking nit, no need to block on that. One remaining comment was GEM-specific options 🤦

@pstibrany pstibrany enabled auto-merge (squash) March 21, 2022 09:16
@pstibrany pstibrany merged commit 8e34a5f into main Mar 21, 2022
@pstibrany pstibrany deleted the dimitar/gem-specific-conversions branch March 21, 2022 09:27
pracucci pushed a commit that referenced this pull request Mar 21, 2022
* Add GEM-specific conversions

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

* Update tests for GEM

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

* Update GEM descriptor

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

* Update descriptors with hidden fields

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

* Set instance-ids for GEM's admin-api too

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>

* Make tests pass & move GEM removed options to gem.go

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
@pracucci pracucci mentioned this pull request Mar 21, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants