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

set value required in constant_keyword field type #419

Merged
merged 2 commits into from
Jul 17, 2024

Conversation

kkewwei
Copy link
Contributor

@kkewwei kkewwei commented Jul 15, 2024

Description

set value required in constant_keyword field type

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].

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.

@kkewwei kkewwei force-pushed the constant_keyword branch 2 times, most recently from 2968b8a to 96cf20d Compare July 15, 2024 15:09
@kkewwei kkewwei changed the title fix constant_keyword field type set value required in constant_keyword field type Jul 15, 2024
Copy link
Contributor

github-actions bot commented Jul 15, 2024

Changes Analysis

Commit SHA: 4762fd2
Comparing To SHA: 6627ca7

API Changes

Summary


└─┬Components
  └─┬_common.mapping:ConstantKeywordProperty
    └─┬ALLOF
      ├──[➕] required (33544:15)❌ 
      └─┬value
        └──[🔀] type (33537:21)❌ 

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

Report

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

API Coverage

Before After Δ
Covered (%) 483 (47.31 %) 483 (47.31 %) 0 (0 %)
Uncovered (%) 538 (52.69 %) 538 (52.69 %) 0 (0 %)
Unknown 24 24 0

@dblock
Copy link
Member

dblock commented Jul 15, 2024

Looks like tests are failing.

Organization-wise I wonder where we should put this test. Is this a _doc or mapping thing? Maybe we want to create subfolders accordingly under tests? WDYT?

tests/mapping/constant_keyword.yaml Outdated Show resolved Hide resolved
spec/schemas/_common.mapping.yaml Show resolved Hide resolved
spec/schemas/_common.mapping.yaml Show resolved Hide resolved
tests/mapping/constant_keyword.yaml Outdated Show resolved Hide resolved
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.

Looks like 2.0 test failed. Which version was this added in? You will need a version: '>= ...' in the test.

Also add a line to CHANGELOG. This is a fix for the constant_keyword mapping type.

@kkewwei kkewwei force-pushed the constant_keyword branch 2 times, most recently from 4e484ef to 6a0294f Compare July 17, 2024 02:33
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.

One nit for CHANGELOG.

The 2.16 daily is failing to startup, I'll look into it.

https://github.com/opensearch-project/opensearch-api-specification/actions/runs/9967302688/job/27562098545

CHANGELOG.md Outdated Show resolved Hide resolved
@kkewwei kkewwei force-pushed the constant_keyword branch 2 times, most recently from 6a3ee29 to 13c7ad0 Compare July 17, 2024 13:44
@kkewwei
Copy link
Contributor Author

kkewwei commented Jul 17, 2024

@dblock, The source code has the bug( opensearch-project/OpenSearch#14651), is this caused by the bug?

@dblock
Copy link
Member

dblock commented Jul 17, 2024

@dblock, The source code has the bug( opensearch-project/OpenSearch#14651), is this caused by the bug?

Dailies don't have all plugins when they fail to build, looks like today security plugin didn't work, so the version on docker hub doesn't have security/SSL, and that's what's failing. I locked it to a specific revision in #431 and we can update it regularly.

@dblock
Copy link
Member

dblock commented Jul 17, 2024

Merging since the 2.16 problem is unrelated. Thanks @kkewwei!

@dblock dblock merged commit 7e1a233 into opensearch-project:main Jul 17, 2024
9 of 11 checks passed
@kkewwei kkewwei deleted the constant_keyword branch July 17, 2024 14:43
@dblock
Copy link
Member

dblock commented Jul 17, 2024

@kkewwei Did I make a mistake merging this? This test is not supposed to work in 2.15 either because of opensearch-project/OpenSearch#14651 and we have to wait for your fix to be merged there? I can revert.

dblock added a commit that referenced this pull request Jul 17, 2024
dblock added a commit that referenced this pull request Jul 17, 2024
@kkewwei kkewwei restored the constant_keyword branch July 17, 2024 15:07
nhtruong pushed a commit that referenced this pull request Jul 17, 2024
@dblock
Copy link
Member

dblock commented Jul 17, 2024

@kkewwei Reopen this when the bug is fixed with >= 2.16 (will need an update of the first image that has the fix in https://github.com/opensearch-project/opensearch-api-specification/blob/main/.github/workflows/test-spec.yml#L29).

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