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

renamed put pipeline to create ingest pipeline #399

Merged
merged 8 commits into from
Nov 10, 2023

Conversation

maddox05
Copy link
Contributor

@maddox05 maddox05 commented Oct 25, 2023

Description

I have renamed the fields put-pipeline and all according names to create ingest pipeline.

Issues Resolved

Solves issue #366

Testing

No testing should be needed for renamed variables/classes.

Looked over by me and others to make sure 100% I got the name correct ( will be looked over again in PR )


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.

@maddox05
Copy link
Contributor Author

Ok I would like to talk more about this in our meeting, but I think I understand where I went wrong with how your test cases work.

@IanHoang
Copy link
Collaborator

@maddox05 Nice! To fix the unittests, you just need to update lines 2248's and 2265's error messages in runner_test.py by replacing put-pipeline with create-ingest-pipeline. Currently, those unit tests are expecting the term put-pipeline in the raised error messages but are actually receiving create-ingest-pipeline.

Copy link
Collaborator

@IanHoang IanHoang left a comment

Choose a reason for hiding this comment

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

Thanks for the quick changes! Just left a comment on how to quickly fix the unittests

@maddox05
Copy link
Contributor Author

maddox05 commented Oct 31, 2023

could not find how [opensearch.ingest.put_pipeline.return_value = as_future()] uses put_pipeline so I changed it to [opensearch.ingest.create_ingest_pipeline.return_value = as_future()], as well as fixing the test cases naming.

@IanHoang
Copy link
Collaborator

IanHoang commented Nov 2, 2023

opensearch.ingest.put_pipeline.return_value = as_future()

@maddox05 This line is included in the unittests but it's referring to the opensearchpy library's put-pipeline and doesn't need to be changed. See the following snippet of code from opensearchpy repository:
https://github.com/opensearch-project/opensearch-py/blob/0d8a23dd78803d2712aa7d8e92663b0953158ba1/opensearchpy/client/ingest.py#L62

The following line occurs twice in runner_test.py:

        with self.assertRaisesRegex(exceptions.DataError,
                                    "Parameter source for operation 'put-pipeline' did not provide the mandatory parameter 'body'. "
                                    "Add it to your parameter source and try again."):

This is what's causing the unittests to fail. The test is expecting an error message that includes "put-pipeline" but it's getting an error message that includes "create-ingest-pipeline". In order to fix the unittests, we just need to replace "put-pipeline" in the error message above to "create-ingest-pipeline".


@mock.patch("opensearchpy.OpenSearch")
@run_async
async def test_param_id_mandatory(self, opensearch):
opensearch.ingest.put_pipeline.return_value = as_future()
opensearch.ingest.create_ingest_pipeline.return_value = as_future()
Copy link
Collaborator

Choose a reason for hiding this comment

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

We won't want to change these because this is an opensearchpy function call. We're only changing occurrences of "put-pipeline" in OSB. Let me know if you have more questions about this and we can discuss this on a call or during office hours.

@@ -1101,7 +1101,7 @@ async def __call__(self, opensearch, params):
)

def __repr__(self, *args, **kwargs):
return "put-pipeline"
return opensearch.ingest.put_pipeline.return_value = as_future()
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should change this line back as it might be impacting the CI runs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why thats left, thought I changed all of them. will change today, sorry about that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No worries!

@IanHoang IanHoang merged commit e73664a into opensearch-project:main Nov 10, 2023
7 of 8 checks passed
IanHoang pushed a commit to IanHoang/opensearch-benchmark that referenced this pull request Jan 9, 2024
@IanHoang IanHoang mentioned this pull request Jan 9, 2024
1 task
gkamat pushed a commit that referenced this pull request Jan 10, 2024
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.

2 participants