Fix ES reset command in Makefile

[pcapriotti]

Fix command for resetting ES index in the Makefile

Checklist

• No changelog entry
• Read and follow the PR guidelines

[mdimjasevic]

Do these directory names have to be updated elsewhere too? The results of ag directory_test in the repo gives me:

services/brig/brig.integration.yaml
14:  index: directory_test

services/brig/src/Brig/Index/Options.hs
116:      _esIndex = ES.IndexName "directory_test",
316:              (progDesc "Delete and re-create the ES user index. Only works on a test index (directory_test).")

hack/helm_vars/wire-server/values.yaml.gotmpl
26:    index: directory_test
47:      index: directory_test

tools/db/find-undead/src/Options.hs
87:    <*> strOption (long "es-index" <> value "directory_test")

Makefile
273:    ./dist/brig-index reset --elasticsearch-index directory_test --elasticsearch-server http://localhost:9200 > /dev/null
274:    ./dist/brig-index reset --elasticsearch-index directory_test2 --elasticsearch-server http://localhost:9200 > /dev/null

[pcapriotti]

That's before this patch, right? What else do you see that needs to be updated?

[mdimjasevic]

That's before this patch, right? What else do you see that needs to be updated?

Sure, but most of the search results are there after the patch too. My hunch is that if you change the directory name in Makefile, it should be done in other places too, otherwise they will search for a wrong directory. But this is just me grepping for a literal string; I haven't actually dived into the semantics of the code using it.

[pcapriotti]

I don't understand. What is the information that you're inferring from "grepping for a literal string"? I don't see any place that needs to be changed, except for the last line, which is indeed what this PR changed. Do you see anything else?

[mdimjasevic]

Strings "directory_test" and "directory_test2" seem very specific and appear in very few places (unlike e.g. "unless", "for" and other strings that appear frequently throughout the codebase), suggesting that their occurrences outside the Makefile are related to what you're changing in the Makefile. I could be wrong and maybe really e.g. hack/helm_vars/wire-server/values.yaml.gotmpl needs no change, but I haven't tried to understand what is going on in that file so I can't say for sure it is wrong to keep that file intact.

[smatting]

#3062 defined the naming of the index. The name for the index is indeed directory2_test:

wire-server/services/run-services

Line 162 in bbb3aeb

data['elasticsearch']['index'] = f"directory{suffix}_test"

[jschaul]

Let's not change these names if not needed.

In production we use cassandra keyspaces brig and elasticsearch indices directory. For everything local we use keyspace brig_test and index directory_test.

This is intentional. The reason is one more failsafe against people port-forwarding to staging or production, and forgetting about that, but using the same ports as services on localhost (e.g. :9042 for cassandra). If names are different, disasters like full database deletion can be averted.
This naming trick has already at least saved us once.

Feel free to add a comment somewhere.

[pcapriotti]

@jschaul I think you're misunderstanding what this does. The reset command hardcodes a _test suffix (for exactly the reasons you describe), and the --elasticsearch-index-prefix option adds a prefix to that. Your command would result in directory_test_test and directory_test2_test as index names.

I hope this clarifies it. The PR shouldn't really be controversial.

[pcapriotti]

Strings "directory_test" and "directory_test2" seem very specific and appear in very few places (unlike e.g. "unless", "for" and other strings that appear frequently throughout the codebase), suggesting that their occurrences outside the Makefile are related to what you're changing in the Makefile. I could be wrong and maybe really e.g. hack/helm_vars/wire-server/values.yaml.gotmpl needs no change, but I haven't tried to understand what is going on in that file so I can't say for sure it is wrong to keep that file intact.

Nothing else needs to be changed, as far as I can tell. In fact, this is not about "changing" anything. It just fixes the command in the reset target, which is just broken, as is. After the patch, it will be the same as the one in the migrate target, which is correct.

I'm not sure why such a trivial patch is sparking so much discussion.

[fisx]

I've just disabled the offending make rule in #3143. if that gets merged, I'll add a commit here to re-enable it.

[jschaul]

@jschaul I think you're misunderstanding what this does. The reset command hardcodes a _test suffix (for exactly the reasons you describe), and the --elasticsearch-index-prefix option adds a prefix to that. Your command would result in directory_test_test and directory_test2_test as index names.

I hope this clarifies it. The PR shouldn't really be controversial.

Ah, I'm sorry. Okay, I didn't know the reset was hardcoding the _test, it's not obvious.
Taking back my criticisim! Sorry!