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

Add strict_allow_templates option for the dynamic mapping parameter #408

Merged
merged 8 commits into from
Jul 23, 2024

Conversation

gaobinlong
Copy link
Contributor

@gaobinlong gaobinlong commented Jul 11, 2024

Description

Add strict_allow_templates option for the dynamic mapping parameter, and remove the unsupported option runtime.

Issues Resolved

#407

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: gaobinlong <gbinlong@amazon.com>
Copy link
Contributor

github-actions bot commented Jul 11, 2024

Changes Analysis

Commit SHA: 15a9826
Comparing To SHA: 34ff292

API Changes

Summary


└─┬Components
  └─┬_common.mapping:DynamicMapping
    ├──[➕] enum (33845:11)
    └──[➖] enum (33844:11)❌ 

Document Element Total Changes Breaking Changes
components 2 1
  • BREAKING Changes: 1 out of 2
  • Removals: 1
  • Additions: 1
  • Breaking Removals: 1

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/10061341381/artifacts/1730976728

API Coverage

Before After Δ
Covered (%) 490 (47.99 %) 490 (47.99 %) 0 (0 %)
Uncovered (%) 531 (52.01 %) 531 (52.01 %) 0 (0 %)
Unknown 24 24 0

Signed-off-by: gaobinlong <gbinlong@amazon.com>
@gaobinlong
Copy link
Contributor Author

The code PR of strict_allow_templates will be released in 2.16.0, so do we need to do some version check in this repo?

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Please add a test into tests. You should be able to write one for the runtime change (it's a bug!), but for the new parameter you'll need to run against a newer version of OpenSearch locally.

CHANGELOG.md Outdated
@@ -38,6 +38,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Added `concurrent_query_*` and `search_idle_reactivate_count_total` fields to `SearchStats` ([#395](https://github.com/opensearch-project/opensearch-api-specification/pull/395))
- Added `remote_store` to `TranslogStats` ([#395](https://github.com/opensearch-project/opensearch-api-specification/pull/395))
- Added `file` to `/_cache/clear` and `/{index}/_cache/clear` ([#396](https://github.com/opensearch-project/opensearch-api-specification/pull/396))
- Add strict_allow_templates option for the dynamic mapping parameter ([#408](https://github.com/opensearch-project/opensearch-api-specification/pull/408))
Copy link
Member

Choose a reason for hiding this comment

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

Quote strict_allow_templates.

@dblock
Copy link
Member

dblock commented Jul 11, 2024

The code PR of strict_allow_templates will be released in 2.16.0, so do we need to do some version check in this repo?

Yes, this is #398, I have #409 ready, but still have to write a way to only run some tests against x.y.z version.

#410 will do what you want, specify version: '>= 2.16'.

Signed-off-by: gaobinlong <gbinlong@amazon.com>
Signed-off-by: gaobinlong <gbinlong@amazon.com>
Signed-off-by: gaobinlong <gbinlong@amazon.com>
@gaobinlong
Copy link
Contributor Author

The failed test seems related to the env, the test case works well locally.
image

@dblock
Copy link
Member

dblock commented Jul 17, 2024

The failed test seems related to the env, the test case works well locally. image

I think 2.16 daily isn't starting, I'll look.

@dblock
Copy link
Member

dblock commented Jul 23, 2024

Update CI to a newer 2.16 build that has your feature and it should all go 🟢 ?

https://github.com/opensearch-project/opensearch-api-specification/blob/main/.github/workflows/test-spec.yml#L29

@gaobinlong
Copy link
Contributor Author

Update CI to a newer 2.16 build that has your feature and it should all go 🟢 ?

https://github.com/opensearch-project/opensearch-api-specification/blob/main/.github/workflows/test-spec.yml#L29

I think so.

@dblock
Copy link
Member

dblock commented Jul 23, 2024

Update CI to a newer 2.16 build that has your feature and it should all go 🟢 ?
https://github.com/opensearch-project/opensearch-api-specification/blob/main/.github/workflows/test-spec.yml#L29

I think so.

I mean you can do it in this PR and we can merge.

Signed-off-by: gaobinlong <gbinlong@amazon.com>
Copy link
Contributor

Spec Test Coverage Analysis

Total Tested
514 114 (22.18 %)

@gaobinlong
Copy link
Contributor Author

@dblock I've updated the image version, now all checks have passed yet.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Great job.

@dblock dblock merged commit 9d2766f into opensearch-project:main Jul 23, 2024
12 checks passed
@dblock
Copy link
Member

dblock commented Jul 23, 2024

If you want to do more, maybe move the template tests into a subfolder, so tests/indices/mapping/dynamic.yaml and add some more tests for other mapping options?

@gaobinlong
Copy link
Contributor Author

If you want to do more, maybe move the template tests into a subfolder, so tests/indices/mapping/dynamic.yaml and add some more tests for other mapping options?

Sure, I'll do that later.

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