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

Added document lifecycle guide & sample code. #545

Conversation

Djcarrillo6
Copy link
Contributor

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
Copy link

codecov bot commented Oct 19, 2023

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
Copy link
Contributor Author

@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
Copy link
Collaborator

saimedhi commented Oct 24, 2023

@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!

Copy link
Collaborator

@saimedhi saimedhi left a comment

Choose a reason for hiding this comment

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

@Djcarrillo6, Sample code tested and working. Please make the below changes and also mention guide link in the user_guide. Thank you :)

guides/document_lifecycle.md Outdated Show resolved Hide resolved
guides/document_lifecycle.md Outdated Show resolved Hide resolved
guides/document_lifecycle.md Outdated Show resolved Hide resolved
guides/document_lifecycle.md Outdated Show resolved Hide resolved
guides/document_lifecycle.md Outdated Show resolved Hide resolved
@Djcarrillo6 Djcarrillo6 force-pushed the fix/issue352/document-lifecycle-guide-and-sample branch from 1c0722f to c8a3b00 Compare October 25, 2023 08:20
Signed-off-by: Djcarrillo6 <[email protected]>

Added a guide on making raw JSON REST requests. (opensearch-project#542)

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

Added document lifecycle guide & sample code.

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

Updated CHANGELOG

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

Added support for AWS Sigv4 for UrlLib3. (opensearch-project#547)

* WIP: Added support for AWS Sigv4 for UrlLib3.

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

* Refactored common implementation.

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

* Added sigv4 samples.

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

* Updated CHANGELOG.

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

* Add documentation.

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

* Use the correct class in tests.

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

* Renamed samples.

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

* Split up requests and urllib3 unit tests.

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

* Rename AWSV4Signer.

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

* Clarified documentation of when to use Urllib3AWSV4SignerAuth vs. RequestHttpConnection.

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

* Move fetch_url inside the signer class.

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

* Added unit test for Urllib3AWSV4SignerAuth adding headers.

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

* Added unit test for signing to include query string.

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

---------

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

Remove support for Python 2.x. (opensearch-project#548)

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

Fixed guide & added link in USER_GUIDE.md

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

Added support for AWS Sigv4 for UrlLib3. (opensearch-project#547)

* WIP: Added support for AWS Sigv4 for UrlLib3.

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

* Refactored common implementation.

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

* Added sigv4 samples.

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

* Updated CHANGELOG.

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

* Add documentation.

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

* Use the correct class in tests.

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

* Renamed samples.

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

* Split up requests and urllib3 unit tests.

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

* Rename AWSV4Signer.

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

* Clarified documentation of when to use Urllib3AWSV4SignerAuth vs. RequestHttpConnection.

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

* Move fetch_url inside the signer class.

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

* Added unit test for Urllib3AWSV4SignerAuth adding headers.

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

* Added unit test for signing to include query string.

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

---------

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

Remove support for Python 2.x. (opensearch-project#548)

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

Fixed guide & added link in USER_GUIDE.md opensearch-project#3

Signed-off-by: Djcarrillo6 <[email protected]>
@Djcarrillo6 Djcarrillo6 force-pushed the fix/issue352/document-lifecycle-guide-and-sample branch from 25e15bb to 200545c Compare October 25, 2023 08:30
@Djcarrillo6
Copy link
Contributor Author

@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
Copy link
Collaborator

@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.

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.

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.

dblock
dblock previously approved these changes Oct 25, 2023
@dblock
Copy link
Member

dblock commented Oct 25, 2023

@saimedhi you good with this one? merge?

CHANGELOG.md Show resolved Hide resolved
USER_GUIDE.md Outdated Show resolved Hide resolved
guides/document_lifecycle.md Outdated Show resolved Hide resolved
@Djcarrillo6
Copy link
Contributor Author

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 Djcarrillo6 force-pushed the fix/issue352/document-lifecycle-guide-and-sample branch from 82f32b3 to 0d760ad Compare October 27, 2023 20:47
@Djcarrillo6 Djcarrillo6 requested a review from saimedhi October 27, 2023 20:50
Signed-off-by: Djcarrillo6 <[email protected]>
@Djcarrillo6 Djcarrillo6 force-pushed the fix/issue352/document-lifecycle-guide-and-sample branch from 3f30f25 to 15ea292 Compare October 27, 2023 20:52
@Djcarrillo6
Copy link
Contributor Author

@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
Copy link
Collaborator

@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 Djcarrillo6 force-pushed the fix/issue352/document-lifecycle-guide-and-sample branch from d716c2a to 3094858 Compare October 27, 2023 21:37
@@ -157,6 +157,7 @@ print(response)
- [Advanced Index Actions](guides/advanced_index_actions.md)
- [Making Raw JSON REST Requests](guides/json.md)
- [Connection Classes](guides/connection_classes.md)
- [Document Lifcycle](guides/document_lifecycle.md)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Lifecycle

Copy link
Collaborator

@saimedhi saimedhi left a comment

Choose a reason for hiding this comment

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

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.

@Djcarrillo6
Copy link
Contributor Author

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

@Djcarrillo6 Djcarrillo6 deleted the fix/issue352/document-lifecycle-guide-and-sample branch October 30, 2023 21:30
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.

4 participants