Remove references to cortex #6015
Conversation
| @@ -105,7 +105,7 @@ func (c *BlobStorageConfig) RegisterFlags(f *flag.FlagSet) { | |||
| // RegisterFlagsWithPrefix adds the flags required to config this to the given FlagSet | |||
| func (c *BlobStorageConfig) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) { | |||
| f.StringVar(&c.Environment, prefix+"azure.environment", azureGlobal, fmt.Sprintf("Azure Cloud environment. Supported values are: %s.", strings.Join(supportedEnvironments, ", "))) | |||
| f.StringVar(&c.ContainerName, prefix+"azure.container-name", "cortex", "Name of the blob container used to store chunks. This container must be created before running cortex.") | |||
| f.StringVar(&c.ContainerName, prefix+"azure.container-name", "loki", "Name of the blob container used to store chunks. This container must be created before running cortex.") | |||
There was a problem hiding this comment.
Should the last parameter string literal also say "Loki" instead of "Cortex?"
There was a problem hiding this comment.
yes, it should. do you want to include that in your bigger docs PR or should I make another PR for this change?
There was a problem hiding this comment.
I'll handle making the change when I put up a PR to remove "cortex" from several places in the docs.
|
@trevorwhitney @owen-d I must be misunderstanding something here, so could you help me out with why this change is valid? When I looked at the code previously I assumed that although it'd have been nice to change, by doing so you'd be breaking anyone already using this storage type? Also wouldn't you need to set it consistently in the other place the same setting is referenced (see below)? I think this is currently incorrect but not a bug as I can't see it actually used anywhere from the second location. loki/pkg/storage/bucket/azure/config.go Line 31 in c76565a I normalised the two lines in #5891 which is where my interest comes due to a rebase conflict. |
|
@stevehipwell you are correct that this represents a breaking change, I will make sure to add to the upgrading guide that if you are relying on this default container name you will have to specify it when doing an upgrade. Thanks for catching that! I don't think we need to update the other location, as the goal of this task was to remove references to "Cortex", since we no longer depend on it. Having these flags defined in 2 places is the result of some technical debt we have yet to pay down. We used to rely on the clients from Cortex for the ruler, but then implemented our own for chunks. When we forked some of the Cortex code while removing that dependency, we ended up with 2 different definitions for these clients. Ideally, I would like to see these definitions moved out of loki, probably to dskit, so we could share them between loki and mimir. However, that is why one references cortex and the other does not. |
|
@trevorwhitney I've aligned the other default in #5891 as I believe they're coming from the same input? As I don't think the second instance is ever used it might be best to remove it from the code unless I'm missing something? |
|
@stevehipwell I think you're spot on, that can be removed, but let's do that in a separate PR. Thanks! |
|
@trevorwhitney could you take a look at #5891 to see what I've done there? |
What this PR does / why we need it:
Found a few stale references to cortex to remove.