-
Notifications
You must be signed in to change notification settings - Fork 13
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
[teraslice, elasticsearch-store] opensearch2 mappings missing dynamics: false field #3808
Conversation
fb218cf
to
26e30b5
Compare
…arness doesn't use elasticsearchApi.
packages/elasticsearch-store/src/elasticsearch-client/method-helpers/helper-utils.ts
Show resolved
Hide resolved
it('should have a jobs index with dynamic mapping false', async () => { | ||
const mapping = await terasliceHarness.client.indices.getMapping({ index: '*__jobs' }); | ||
const indexName = Object.keys(mapping)[0]; | ||
const searchVersion = (await terasliceHarness.client.info()).version.number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(await terasliceHarness.client.info()).version.number;
is not very readable and its uneeded. The client already has that information on it so there is no need to make the call either through terasliceHarness.client.distributionMeta
though the types wont like that or through terasliceHarness.client.__meta
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know that. Thanks
@@ -111,5 +111,6 @@ export { | |||
MINIO_ACCESS_KEY, | |||
MINIO_SECRET_KEY, | |||
ENCRYPT_MINIO, | |||
ROOT_CERT_PATH | |||
ROOT_CERT_PATH, | |||
TEST_OPENSEARCH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can also pull the version from the enviroment not only just TEST_OPENSEARCH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my use case I would need to expose both elasticsearch and opensearch versions, so I'll stick will getting the version from client.__meta.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM besides some simple test changes
This PR makes the following changes:
ensureNoTypeInMapping()
to copydynamic
property when removing type from mapping.method-helpers-spec.ts
and adds unit tests forensureNoTypeInMapping()
.storage-spec.ts
to confirm that the jobs state storage index mapping is created withdynamic
set tofalse
.Ref: #3809