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

Setting #621

Closed
wants to merge 23 commits into from
Closed

Setting #621

wants to merge 23 commits into from

Conversation

brunoLloret
Copy link
Contributor

Description

  • Removed extra docker-compose file
  • Removed script_language and shard_store tests due to API unfamiliarity
  • Fixed DCO by amending commit with -s flag
  • Rebased and squashed commits for easier DCO fix
  • Ran linter with npm run lint --fix
  • Ensured all remaining tests pass

Note to reviewers:

  • DCO has been fixed
  • Commits have been squashed
  • Linter has been run
  • All current tests are passing
  • Remaining tests were already in tests/default directory (?)
  • I will explore more the API to move ahead with more substantial tests, any hints or further indications are welcomed!

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.

@brunoLloret
Copy link
Contributor Author

I will sign off the unsigned commit and resubmit

@brunoLloret brunoLloret force-pushed the setting branch 2 times, most recently from 2b15005 to e0de9ea Compare October 17, 2024 17:44
Xtansia and others added 16 commits October 17, 2024 16:14
* Allow shortcut specifying InlineScript as just a string

Signed-off-by: Thomas Farr <[email protected]>

* Add test

Signed-off-by: Thomas Farr <[email protected]>

* Allow arbitrary script language

Signed-off-by: Thomas Farr <[email protected]>

---------

Signed-off-by: Thomas Farr <[email protected]>
Signed-off-by: Bruno Lloret <[email protected]>
Signed-off-by: dblock <[email protected]>
Signed-off-by: Bruno Lloret <[email protected]>
* fix spec snapshot endpoint

Signed-off-by: Tatsuya Kawakami <[email protected]>

* fix lint and add changelog

Signed-off-by: Tatsuya Kawakami <[email protected]>

---------

Signed-off-by: Tatsuya Kawakami <[email protected]>
Signed-off-by: Bruno Lloret <[email protected]>
* update with accurate numeric type for common, core

Signed-off-by: amberzsy <[email protected]>

* add changelog

Signed-off-by: amberzsy <[email protected]>

* update explian value field with 'General Number'

Signed-off-by: amberzsy <[email protected]>

---------

Signed-off-by: amberzsy <[email protected]>
Signed-off-by: Bruno Lloret <[email protected]>
- Added tests for core endpoints: _refresh, _remote/info, _script_context
- Added test for indices endpoint: _template
- Removed tests for _script_language and _shard_stores due to API unfamiliarity
- Cleaned up local environment files

Signed-off-by: Bruno Lloret <[email protected]>
plain GETs for ./tests/default/_core/ refresh.yaml, remote_info.yaml, script_context.yaml, and script_language.yaml; for ./tests/default/indices/ shard_stores.yaml, and template.yaml

gitignore

Remove .env from tracking and add it to .gitignore

Add local Docker Compose file to .gitignore

Signed-off-by: Bruno Lloret <[email protected]>
* fix snapshot restore and recovery endpoint

Signed-off-by: Tatsuya Kawakami <[email protected]>

* add changelog

Signed-off-by: Tatsuya Kawakami <[email protected]>

* Fixed index naming, description and changelog

Signed-off-by: Tatsuya Kawakami <[email protected]>

---------

Signed-off-by: Tatsuya Kawakami <[email protected]>
Signed-off-by: Bruno Lloret <[email protected]>
* Edit msearch parameters descriptions

Signed-off-by: Archer <[email protected]>

* Apply suggestions from code review

Co-authored-by: Nathan Bower <[email protected]>
Signed-off-by: Naarcha-AWS <[email protected]>

---------

Signed-off-by: Archer <[email protected]>
Signed-off-by: Naarcha-AWS <[email protected]>
Co-authored-by: Nathan Bower <[email protected]>
Signed-off-by: Bruno Lloret <[email protected]>
plain GETs for ./tests/default/_core/ refresh.yaml, remote_info.yaml, script_context.yaml, and script_language.yaml; for ./tests/default/indices/ shard_stores.yaml, and template.yaml

gitignore

Remove .env from tracking and add it to .gitignore

BAdd local Docker Compose file to .gitignore

Signed-off-by: Bruno Lloret <[email protected]>
… script_context.yaml, and script_language.yaml; for ./tests/default/indices/ shard_stores.yaml, and template.yaml

Signed-off-by: Bruno Lloret <[email protected]>
Signed-off-by: Bruno Lloret <[email protected]>
Copy link
Contributor

Changes Analysis

Commit SHA: 3461acc
Comparing To SHA: d2733fe

API Changes

Summary

NO CHANGES

Report

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

API Coverage

Before After Δ
Covered (%) 588 (57.59 %) 588 (57.59 %) 0 (0 %)
Uncovered (%) 433 (42.41 %) 433 (42.41 %) 0 (0 %)
Unknown 29 29 0

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 a test failed, an API was likely introduced later than 1.3.7 and a test needs a version: >= ... tag. Make sure to confirm it's the case.

ERROR   _core/script_language.yaml (tests/default/_core/script_language.yaml)
    ERROR   CHAPTERS
        ERROR   Retrieve information about available script languages.
            PASSED  REQUEST BODY
            ERROR   RESPONSE STATUS (Expected status 200, but received 500: application/json. Internal Server Error)
            SKIPPED RESPONSE PAYLOAD BODY
            SKIPPED RESPONSE PAYLOAD SCHEMA
            SKIPPED RESPONSE OUTPUT VALUES

I think you should make this PR just about the new tests and remove all the setup/package.json and related changes.

There's a lot of commits in this PR for what ends up being a small change, you should squash them into 1 commit in this PR to clean things up and make git history easier to read.

package.json Outdated
},
"devDependencies": {
"dotenv": "^16.4.5",
"eslint-plugin-yaml": "^1.0.3"
Copy link
Member

Choose a reason for hiding this comment

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

This seems out of place. This project doesn't release a JavaScript library, so move these into dependencies to be consistent.

tests/default/_core/refresh.yaml Outdated Show resolved Hide resolved
.cspell Outdated
@@ -84,6 +84,7 @@ lucene
Lucene
lycheeverse
marvinpinto
memlock
Copy link
Member

Choose a reason for hiding this comment

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

Are those used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they were on my local docker-compose.yaml, got it!


# coverage output
/coverage/

Copy link
Member

Choose a reason for hiding this comment

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

Was this accidentally deleted? We want this.

@@ -118,6 +118,7 @@ export default [
},
rules: {
'yml/no-empty-document': 'off',
'yml/no-empty-mapping-value': 'off',
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

linter was complaining when I left a value empty in one of the basic tests I was writing, I will delete it though and make sure it makes sense for future tests

brunoLloret and others added 4 commits October 18, 2024 08:20
Signed-off-by: Bruno Lloret <[email protected]>
Signed-off-by: Bruno Lloret <[email protected]>
Co-authored-by: Daniel (dB.) Doubrovkine <[email protected]>
Signed-off-by: Bruno Lloret <[email protected]>
Signed-off-by: Bruno Lloret <[email protected]>
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.

6 participants