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

Adding a guide for making raw json requests using the client #399

Merged
merged 3 commits into from
Nov 21, 2023

Conversation

VachaShah
Copy link
Collaborator

Description

Same as title

Issues Resolved

Closes #395

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.

@VachaShah
Copy link
Collaborator Author

This code is tested against OpenSearch cluster.

guides/json.md Outdated Show resolved Hide resolved
@Jakob3xD Jakob3xD force-pushed the raw-json-guide branch 3 times, most recently from 9734a1f to 3a05262 Compare November 20, 2023 11:47
Jakob3xD
Jakob3xD previously approved these changes Nov 20, 2023
dblock
dblock previously approved these changes Nov 20, 2023
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.

LGTM. Can we add a runnable sample along with this?

@Jakob3xD
Copy link
Collaborator

Can we add a runnable sample along with this?

Not 100% sure what you mean with that.
I updated the code blocks in such a way that if you copy them in the same order you will get a working go file. Similar to the other guids. See the main() function and the started exaple() function in the Setup section. Which I btw. forgot to add to the list on the top.

Signed-off-by: Vacha Shah <vachshah@amazon.com>
Signed-off-by: Vacha Shah <vachshah@amazon.com>
@Jakob3xD Jakob3xD dismissed stale reviews from dblock and themself via bc8d1c1 November 20, 2023 12:59
Jakob3xD
Jakob3xD previously approved these changes Nov 20, 2023
@dblock
Copy link
Member

dblock commented Nov 20, 2023

We had a lot of contributions where code didn't work when copy-pasted, so in other clients we started adding samples that users can "just run", e.g. https://github.com/opensearch-project/opensearch-py/tree/main/samples. WDYT?

Copy link

codecov bot commented Nov 20, 2023

Codecov Report

Merging #399 (2e78c3d) into main (81200d8) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #399   +/-   ##
=======================================
  Coverage   57.25%   57.25%           
=======================================
  Files         315      315           
  Lines        9820     9820           
=======================================
  Hits         5622     5622           
  Misses       2904     2904           
  Partials     1294     1294           
Flag Coverage Δ
integration 50.82% <ø> (ø)
unit 12.82% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@Jakob3xD
Copy link
Collaborator

Added samples for all guids, as I already had them when I updated the guids.

Once this is merge we could replace the code blocks of the guids with permanent links to the files instead of copy paste the code?

Signed-off-by: Jakob Hahn <jakob.hahn@hetzner.com>
@dblock
Copy link
Member

dblock commented Nov 21, 2023

Once this is merge we could replace the code blocks of the guids with permanent links to the files instead of copy paste the code?

You could, but other clients don't really do that. I see guides kept to something concise and copy-pasted and working samples. Ideally samples should be compiled and run as integration tests and guide reference code, of course, but that needs tools.

@dblock dblock merged commit b132d80 into opensearch-project:main Nov 21, 2023
49 checks passed
@dblock
Copy link
Member

dblock commented Nov 21, 2023

We forgot to link json.md from USER_GUIDE. Maybe in a future PR?

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.

[FEATURE] Make raw JSON REST requests to OpenSearch
3 participants