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

✨Airbyte CDK: add POST method to HttpMocker #34001

Merged
merged 11 commits into from
Jan 10, 2024

Conversation

askarpets
Copy link
Contributor

What

  • Add body parameter to HttpRequest objects
  • Add post method to HttpMocker

Recommended reading order

  1. request.py
  2. mocker.py

🚨 User Impact 🚨

No breaking changes

Pre-merge Actions

Updating the Python CDK

Airbyter

Before merging:

  • Pull Request description explains what problem it is solving
  • Code change is unit tested
  • Build and my-py check pass
  • Smoke test the change on at least one affected connector
    • On Github: Run this workflow, passing --use-local-cdk --name=source-<connector> as options
    • Locally: airbyte-ci connectors --use-local-cdk --name=source-<connector> test
  • PR is reviewed and approved

After merging:

  • Publish the CDK
    • The CDK does not follow proper semantic versioning. Choose minor if this the change has significant user impact or is a breaking change. Choose patch otherwise.
    • Write a thoughtful changelog message so we know what was updated.
  • Merge the platform PR that was auto-created for updating the Connector Builder's CDK version
    • This step is optional if the change does not affect the connector builder or declarative connectors.

@askarpets askarpets self-assigned this Jan 8, 2024
@askarpets askarpets requested a review from a team as a code owner January 8, 2024 13:48
Copy link

vercel bot commented Jan 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Jan 10, 2024 6:28am

@octavia-squidington-iii octavia-squidington-iii added the CDK Connector Development Kit label Jan 8, 2024
@girarda girarda requested a review from maxi297 January 8, 2024 13:50
Copy link
Contributor

@maxi297 maxi297 left a comment

Choose a reason for hiding this comment

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

I'm wondering how we can make sure the UX is good for body as the requests library allows for multiple ways to create a request and it should be easy for the dev to generate it.

My very generic impression is that if a body is provided for a request (which is going to be the case most often then not because devs think in terms of object more than string and that object matching is less flaky/susceptible to formatting than string), we should match as JSON. However, I think we should play with this a little and see the different cases that need to be supported

airbyte-cdk/python/airbyte_cdk/test/mock_http/mocker.py Outdated Show resolved Hide resolved
airbyte-cdk/python/airbyte_cdk/test/mock_http/mocker.py Outdated Show resolved Hide resolved
airbyte-cdk/python/airbyte_cdk/test/mock_http/request.py Outdated Show resolved Hide resolved
@askarpets askarpets requested a review from maxi297 January 9, 2024 17:11
Copy link
Contributor

@maxi297 maxi297 left a comment

Choose a reason for hiding this comment

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

Auto-approving so that @askarpets can merge if he think the latest change is good

_A_RESPONSE,
)

requests.post(_A_URL, params=_SOME_QUERY_PARAMS, headers=_SOME_HEADERS, data=_SOME_REQUEST_BODY_STR)
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that I had to change json to data here as requests would cast _SOME_REQUEST_BODY_STR as "some_request_body" and not some_request_body

actual_request = HttpRequest("mock://test.com/path", headers={"first_header": "value does not match"})
assert not actual_request.matches(request_to_match)

def test_given_same_body_mappings_value_when_matches_then_return_true(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

I moved the tests you had done in test_http_mocker to this class to avoid having duplicates and making sure it's as unit as a unit test can be

@askarpets askarpets merged commit 8305d05 into master Jan 10, 2024
20 checks passed
@askarpets askarpets deleted the airbyte-cdk-tests-support-post-method branch January 10, 2024 10:44
jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024
Co-authored-by: maxi297 <maxime@airbyte.io>
jatinyadav-cc pushed a commit to ollionorg/datapipes-airbyte that referenced this pull request Feb 26, 2024
Co-authored-by: maxi297 <maxime@airbyte.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CDK Connector Development Kit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants