opensearch-project / opensearch-py

Python Client for OpenSearch
https://opensearch.org/docs/latest/clients/python/
Apache License 2.0
359 stars 178 forks source link

Added document lifecycle guide & sample code. #545

Closed Djcarrillo6 closed 1 year ago

Djcarrillo6 commented 1 year ago

Description

Adds API guide for the document lifecycle API(s) & adds sample code file to test the examples.

Issues Resolved

Resolves issue 352 Allow for the closure of open PR 364

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.

OSCI-2023

codecov[bot] commented 1 year ago

Codecov Report

Merging #545 (3094858) into main (bcfef11) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #545   +/-   ##
=======================================
  Coverage   71.10%   71.10%           
=======================================
  Files          85       85           
  Lines        7774     7774           
=======================================
  Hits         5528     5528           
  Misses       2246     2246           
Djcarrillo6 commented 1 year ago

@saimedhi is this WhiteSource Security Check test failure caused by my changes, or is it unrelated to my PR? I see that it's a similar issue I merged a fix for a couple weeks ago. This warning is now suggesting the urllib3 package be bumped to 1.26.18 which is the next patch version up from the 1.26.17 that I previously bumped it to. If it is unrelated, would you like me to open an issue for it?

saimedhi commented 1 year ago

@saimedhi is this WhiteSource Security Check test failure caused by my changes, or is it unrelated to my PR? I see that it's a similar issue I merged a fix for a couple weeks ago. This warning is now suggesting the urllib3 package be bumped to 1.26.18 which is the next patch version up from the 1.26.17 that I previously bumped it to. If it is unrelated, would you like me to open an issue for it?

Hello @Djcarrillo6, I think this PR might be causing the issue because I didn't notice whitesource check failure in other PRs. Could you please try syncing your branch with the main branch to see if it resolves the problem? Thanks!

Djcarrillo6 commented 1 year ago

@saimedhi I made the changes you requested by I am running into an issue each time I try to rebase my commits into 1 single commit. Each time I try to rebase I end up stomping out & causing merge conflicts from commits that aren't coming from me?

This is the command I usually run when I make a change & I want to rebase by git history by squashing my two commits into one, and below is the same conflict error I always get and also the commit hash from a commit msg that isn't from my changes. Maybe it's something I pulled in when I tried to sync with main? Or maybe I am pulling in someone's commits who is working directly on main at the same time I am trying to rebase?

git rebase -i HEAD~2

Auto-merging CHANGELOG.md
CONFLICT (content): Merge conflict in CHANGELOG.md
Auto-merging USER_GUIDE.md
error: could not apply fa8f3a7... Added a guide on making raw JSON REST requests. (#542)
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply fa8f3a7... Added a guide on making raw JSON REST requests. (#542)
saimedhi commented 1 year ago

@saimedhi I made the changes you requested by I am running into an issue each time I try to rebase my commits into 1 single commit. Each time I try to rebase I end up stomping out & causing merge conflicts from commits that aren't coming from me?

This is the command I usually run when I make a change & I want to rebase by git history by squashing my two commits into one, and below is the same conflict error I always get and also the commit hash from a commit msg that isn't from my changes. Maybe it's something I pulled in when I tried to sync with main? Or maybe I am pulling in someone's commits who is working directly on main at the same time I am trying to rebase?

git rebase -i HEAD~2

Auto-merging CHANGELOG.md
CONFLICT (content): Merge conflict in CHANGELOG.md
Auto-merging USER_GUIDE.md
error: could not apply fa8f3a7... Added a guide on making raw JSON REST requests. (#542)
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply fa8f3a7... Added a guide on making raw JSON REST requests. (#542)

Hi @Djcarrillo6, I recommend closing this PR and opening a new one with your changes.

dblock commented 1 year ago

@saimedhi you good with this one? merge?

Djcarrillo6 commented 1 year ago

Don't worry about a single commit thing, we can squash it.

For when you want to do it, the easiest way is to git reset HEAD~1 which undoes your last commit, then git add and git commit --amend which adds it to the previous commit, then git push origin [branch] -f.

Thanks for the guidance @dblock! @saimedhi I will implement the changes you requested(including fixing any of the merge conflicts). I will try Daniel's recommendation for the rebase first, but if for some reason I can't figure it out, then I will open a new PR. I should be able to get this ready for another review this afternoon. Thank you both again!

Djcarrillo6 commented 1 year ago

@saimedhi I see I now have a failing CI/lint check.. The error message says I am missing a license header in my document_lifecycle.md file, however I don't know what that is. I don't believe I have used/seen it in the other guides, but I could totally be misunderstanding the error message too. Do I need to add a license header to my MD file?

VachaShah commented 1 year ago

@saimedhi I see I now have a failing CI/lint check.. The error message says I am missing a license header in my document_lifecycle.md file, however I don't know what that is. I don't believe I have used/seen it in the other guides, but I could totally be misunderstanding the error message too. Do I need to add a license header to my MD file?

Hi @Djcarrillo6! This is from a recent change where this check was added for samples folder (PR #556). The license header should be as can be found here https://github.com/opensearch-project/opensearch-py/blob/main/samples/index_template/index_template_sample.py#L1-L11

Djcarrillo6 commented 1 year ago

Hi @Djcarrillo6, both the guide and the sample look good for approval and merging. However, I noticed some unintended changes in your PR when syncing with the main branch. I recommend creating a new PR.

Okay will do