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

Crud extension #71

Merged
merged 3 commits into from
Nov 14, 2023
Merged

Conversation

Jatin-tec
Copy link
Contributor

Description

This pull request addresses Issue #55 by implementing an example CRUD extension in the samples directory. The extension demonstrating basic CRUD operations on an index will and how to handle CRUD operations using OpenSearch SDK for Python with documentation. This addition serves as a reference for developers looking to create similar extensions, showcasing best practices and usage patterns.

Issues Resolved

Closes #55.

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.

@Jatin-tec Jatin-tec force-pushed the crud-extension branch 2 times, most recently from 01a226f to 820488b Compare October 28, 2023 17:15
@codecov
Copy link

codecov bot commented Oct 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (07c0e55) 100.00% compared to head (8925c52) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #71   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           55        55           
  Lines         1469      1469           
=========================================
  Hits          1469      1469           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

Fantastic work, @Jatin-tec ! Thanks for your contribution!

I left a few comments, but most of them need to wait until we get #45 and #49 fixed, and we can update the sample then.

samples/crud_extension/README.md Outdated Show resolved Hide resolved
samples/crud_extension/__init__.py Outdated Show resolved Hide resolved
samples/crud_extension/crud_handler.py Outdated Show resolved Hide resolved
samples/crud_extension/crud_handler.py Outdated Show resolved Hide resolved
samples/crud_extension/crud_handler.py Outdated Show resolved Hide resolved
@dbwiddis
Copy link
Member

dbwiddis commented Nov 9, 2023

Hey @Jatin-tec thanks for updating the formatting.

Linting is failing on something that I'm not sure how to fix, maybe @dblock can clarify:

samples/crud_extension/crud.py:12: error: Module "crud_extension" has no attribute "CRUDExtension"  [attr-defined]

Also the DCO check is failing. Since you've signed at least one and we'll squash the commits we can work with that but if you can easily retroactively sign it'll make the CI happier.

@dblock
Copy link
Member

dblock commented Nov 10, 2023

@Jatin-tec lmk if you can't figure out the lint problem and I can take a look

@Jatin-tec Jatin-tec force-pushed the crud-extension branch 2 times, most recently from dc638a1 to 0f909c1 Compare November 10, 2023 20:54
@Jatin-tec
Copy link
Contributor Author

Jatin-tec commented Nov 10, 2023

I have incorporated all the suggested changes hope this will resolve the issue and thank you @dbwiddis @dblock for your fantastic support. Your encouragement made my first contribution experience truly amazing and I learned a lot. Thank you!

@dbwiddis dbwiddis force-pushed the crud-extension branch 2 times, most recently from 17c36d4 to 15823a6 Compare November 11, 2023 00:08
@dbwiddis
Copy link
Member

dbwiddis commented Nov 11, 2023

Hey @Jatin-tec thanks again for your work and patience through the process. I think we can take it from here.

I rebased your code with the main branch since GitHub couldn't figure out a merge conflict in the poetry lock file.

CI is still failing on mypy error:

$ poetry run mypy .          
samples/crud_extension/crud.py:12: error: Module "crud_extension" has no attribute "CRUDExtension"  [attr-defined]
Found 1 error in 1 file (checked 129 source files)

I've been looking into this and trying to resolve it. I think the problem is that the file name crud_extension.py matches the directory/module name crud_extension. Evidence:

  • If I delete the line from crud_extension import CRUDExtension and recreate it with a quick fix I get from samples.crud_extension.crud_extension import CRUDExtension which results in a different mypy error "Source file found twice under different module names: "crud_extension.crud_extension" and "samples.crud_extension.crud_extension""
  • Deleting the prefix samples makes mypy happy: from crud_extension.crud_extension import CRUDExtension
  • I'd commit that change, but this still seems to be an extra step not required by the hello extension.

So is there a better fix here?

@Jatin-tec
Copy link
Contributor Author

Hey @Jatin-tec thanks again for your work and patience through the process. I think we can take it from here.

I rebased your code with the main branch since GitHub couldn't figure out a merge conflict in the poetry lock file.

CI is still failing on mypy error:

$ poetry run mypy .          
samples/crud_extension/crud.py:12: error: Module "crud_extension" has no attribute "CRUDExtension"  [attr-defined]
Found 1 error in 1 file (checked 129 source files)

I've been looking into this and trying to resolve it. I think the problem is that the file name crud_extension.py matches the directory/module name crud_extension. Evidence:

* If I delete the line `from crud_extension import CRUDExtension` and recreate it with a quick fix I get `from samples.crud_extension.crud_extension import CRUDExtension` which results in a different mypy error "Source file found twice under different module names: "crud_extension.crud_extension" and "samples.crud_extension.crud_extension""

* Deleting the prefix `samples` makes mypy happy: `from crud_extension.crud_extension import CRUDExtension`

* I'd commit that change, but this still seems to be an extra step not required by the hello extension.

So is there a better fix here?

I can restructure the module layout, I think that should fix this.

@dbwiddis
Copy link
Member

I can restructure the module layout, I think that should fix this.

I suspect simply renaming samples/crud_extension to samples/crud would be sufficient.

Also if you're still iterating, I've handled the consumed content stuff in #76, if you'd like to integrate those changes in your sample I can merge that PR and you can rebase. Otherwise I'm holding off on merging that and can make changes to your code here after we get this merged.

dbwiddis and others added 2 commits November 13, 2023 17:49
Signed-off-by: mend-for-github-com[bot] <mend-for-github-com[bot]@users.noreply.github.com>
Co-authored-by: mend-for-github-com[bot] <50673670+mend-for-github-com[bot]@users.noreply.github.com>
Signed-off-by: Jatin <jatin.kshatriya2821@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
@dbwiddis dbwiddis merged commit 6162c4d into opensearch-project:main Nov 14, 2023
7 checks passed
@dbwiddis
Copy link
Member

Thanks for this contribution, @Jatin-tec! Hope you stick around and do more!

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] Create sample CRUD extension
3 participants