🐛 [low-code] $options shouldn't overwrite values that are already defined #18060

girarda · 2022-10-17T15:23:27Z

What

Parameters passed as $options overwrite hard coded parameters.
For example,

  rates_stream:
    $ref: "*ref(definitions.base_stream)"
    $options:
      name: "rates"
      primary_key: "date"
      path: "/exchangerates_data/latest"
    transformations:
      - type: RemoveFields
        field_pointers:
          - ["rates", "AED"]
      - type: AddFields
        fields:
          - path: ["some_new_path"]
            value: "some new string foobar"

The AddField's path will be "/exchangerates_data/latest" instead of "some_new_path".

The create function shouldn't overwrite parameters that are already set.

See #17957 for more details

How

Check if the parameters are already set before updating them with the value from $options
Add unit tests

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub by running the /publish command described here
After the connector is published, connector added to connector index as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

girarda · 2022-10-17T15:30:49Z

airbyte-cdk/python/unit_tests/sources/declarative/test_create_partial.py

@@ -46,7 +52,7 @@ def test_string_interpolation():
    assert interpolated_string.string == s


-def test_string_interpolation_through_kwargs():
+def test_string_interpolation_through_options():


rename test

girarda · 2022-10-17T15:31:10Z

airbyte-cdk/python/unit_tests/sources/declarative/test_create_partial.py

@@ -33,6 +33,12 @@ def test_pass_parameter_to_create_function():
    assert object.another_param == "B"


+def test_parameter_not_overwritten_by_options():


I confirmed this test fails without the changes to create_partial.py

girarda · 2022-10-17T15:31:14Z

airbyte-cdk/python/unit_tests/sources/declarative/test_factory.py

@@ -682,6 +682,37 @@ def test_add_fields(self):
        ]
        assert expected == component.transformations

+    def test_add_fields_path_in_options(self):


I confirmed this test fails without the changes to create_partial.py

sherifnada

Brilliant, thank you!

girarda · 2022-10-17T17:35:41Z

/publish-cdk dry-run=false

🕑 https://github.com/airbytehq/airbyte/actions/runs/3267291719
✅ https://github.com/airbytehq/airbyte/actions/runs/3267291719

…ined (#18060) * fix * Add missing test * remove prints * extract to method * rename * Add missing test * rename * bump

* Implement ColumnSortButton component * Updates component name; Moves component to ui/Table folder; Refactors formattedMessageId property into using render content as children directly; Removes minor SortIcon component * Update airbyte-webapp/src/App.tsx Co-authored-by: Edmundo Ruiz Ghanem <168664+edmundito@users.noreply.github.com> * Updates next properties: wasActive -> isActive, lowToLarge -> isAscending * Skip psql stop in acceptance test for gke (#18023) * Checks for iterator hasNext element (#18041) * Checks for iterator hasNext element * Fix linter with newline * Add Message Migration to Destination Connection Checks (#17954) * Add Message Migration to Destination Connection Checks * Fix test setup * Update helm release workflow (#18048) * Update workflow * Update trigger rules * fix: Update release workflow with abillity to add tags * Update workflow * Remove unused `airbyte-cli` (#18009) * 🐛 [low-code] $options shouldn't overwrite values that are already defined (#18060) * fix * Add missing test * remove prints * extract to method * rename * Add missing test * rename * bump * Update helm chart comments (#18072) * Update helm charts (#18073) * add test * fix chart.yaml * 16250 Destination Redis: Add SSH support (#17951) * 16250 Destination Redis: Add SSH support * 16250 Resolve port issue * 11679 Bump version * auto-bump connector version Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com> * Bump helm chart version reference to 0.40.20 (#18074) * Bump helm chart version reference to 0.40.20 * remove binary Co-authored-by: xpuska513 <xpuska513@users.noreply.github.com> Co-authored-by: Kyryl Skobylko <xpuska513@gmail.com> * Helm Chart: Create service annotations for airbyte-server (#17932) * Support annotations for airbyte-server as well, update version and update docs. * Fix auto-indent. Co-authored-by: Kyryl Skobylko <xpuska513@gmail.com> * Bmoric/remove dep server worker (#17894) * test [ci skip] * Autogenerated files * Add missing annotation * Remove unused json2Schema block from worker * Move tess * Missing deps and format * Fix test build * TMP * Add missing dependencies * PR comments * Tmp * [ci skip] Tmp * Fix acceptance test and add the seed dependency * Fix build * For diff * tmp * Build pass * make the worker to be on the platform only * fix setting.yaml * Fix pmd * Fix Cron * Add chart * Fix cron * Fix server build.gradle * Fix jar conflict * PR comments * Add cron micronaut environemnt * Updated connector catalog page (#18076) * Move the port forward outside of the main docker-compose (#17864) * Bump Airbyte version from 0.40.14 to 0.40.15 (#17970) Co-authored-by: benmoriceau <benmoriceau@users.noreply.github.com> * 🎉 Source Shopify: Add metafield streams (#17962) * 🎉 Source Shopify: Add metafield streams * Source Shopify: fix unittest * Source Shopify: docs update * Source Shopify: fix backward compatibility test * Source Shopify: fix schemas * Source Shopify: fix state filter * Source Shopify: refactor & optimize * Source Shopify: fix test privileges * Source Shopify: fix stream filter * Source Shopify: fix streams * Source Shopify: update abnormal state * Source Shopify: fix abnormal state streams * Source Shopify: fix streams * updated methods, formated code * Source Shopify: typo fix * auto-bump connector version Co-authored-by: Oleksandr Bazarnov <oleksandr.bazarnov@globallogic.com> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com> * fix check for streams that do not use a stream slicer (#18080) * fix check for streams that do not use a stream slicer * increment version and changelog before publish * tolerate database nulls in webhook operation configs (#18084) * Implement webhook operation in the sync workflow (#18022) Implements the webhook operation as part of the sync workflow. - Introduces the new activity implementation - Updates the various interfaces that pass input to get the relevant configs to the sync workflow - Hooks the new activity into the sync workflow - Passes the webhook configs along into the sync workflow job * Bump helm chart version reference to 0.40.22 (#18077) * Added new "filters" python file, along with a "hash" filter. This can… (#18000) * Added new "filters" python file, along with a "hash" filter. This can be extended to include other custom filters in the future. * Added additional comments * Moved usage of the hash_obj inside the conditional that confirms it exists * Moved the hash function call inside a condition to ensure that it exists * Fixed the application of the salt , so that it does not modify the hash unless it is actually passed in. * Added unit tests to validate new jinja hash functionality * Updated unit test to pass numeric value as a float instead of string * Removed unreferenced import to pytest * Updated version * format * format * format * format * format Co-authored-by: Alexandre Girard <alexandre@airbyte.io> * Bump helm chart version reference to 0.40.24 (#18081) * Bump helm chart version reference to 0.40.24 * Update .gitignore Co-authored-by: benmoriceau <benmoriceau@users.noreply.github.com> Co-authored-by: Kyryl Skobylko <xpuska513@gmail.com> * SATs: allow new records in a sequential read for full refresh test (#17660) * SATs: allow new records in a sequential read for full refresh test * SATs: upd changelog * SATs: change the output when failing full refresh test * SATs: upd according to code review * Source facebook-marketing: remove `pixel` from custom conversions stream (#18045) * #744 source facebook-marketing: rm pixel from custom conversions stream * #744 source fb marketing: upd changelog * #744 source facebook-marketing - add custom_conversions to the test catalog * auto-bump connector version Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com> * #17506 fix klaviyo & marketo expected_records (#18101) Co-authored-by: Edmundo Ruiz Ghanem <168664+edmundito@users.noreply.github.com> Co-authored-by: terencecho <3916587+terencecho@users.noreply.github.com> Co-authored-by: Ryan Fu <ryan.fu@airbyte.io> Co-authored-by: Jimmy Ma <gosusnp@users.noreply.github.com> Co-authored-by: Kyryl Skobylko <xpuska513@gmail.com> Co-authored-by: Evan Tahler <evan@airbyte.io> Co-authored-by: Alexandre Girard <alexandre@airbyte.io> Co-authored-by: Yevhen Sukhomud <suhomud@gmail.com> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: xpuska513 <xpuska513@users.noreply.github.com> Co-authored-by: Prasanth <72515998+sfc-gh-pkommini@users.noreply.github.com> Co-authored-by: Benoit Moriceau <benoit@airbyte.io> Co-authored-by: Amruta Ranade <11484018+Amruta-Ranade@users.noreply.github.com> Co-authored-by: Octavia Squidington III <90398440+octavia-squidington-iii@users.noreply.github.com> Co-authored-by: benmoriceau <benmoriceau@users.noreply.github.com> Co-authored-by: Artem Inzhyyants <36314070+artem1205@users.noreply.github.com> Co-authored-by: Oleksandr Bazarnov <oleksandr.bazarnov@globallogic.com> Co-authored-by: Brian Lai <51336873+brianjlai@users.noreply.github.com> Co-authored-by: Michael Siega <109092231+mfsiega-airbyte@users.noreply.github.com> Co-authored-by: Alexander Marquardt <alexander.marquardt@gmail.com> Co-authored-by: Denys Davydov <davydov.den18@gmail.com>

…ined (airbytehq#18060) * fix * Add missing test * remove prints * extract to method * rename * Add missing test * rename * bump

* Implement ColumnSortButton component * Updates component name; Moves component to ui/Table folder; Refactors formattedMessageId property into using render content as children directly; Removes minor SortIcon component * Update airbyte-webapp/src/App.tsx Co-authored-by: Edmundo Ruiz Ghanem <168664+edmundito@users.noreply.github.com> * Updates next properties: wasActive -> isActive, lowToLarge -> isAscending * Skip psql stop in acceptance test for gke (airbytehq#18023) * Checks for iterator hasNext element (airbytehq#18041) * Checks for iterator hasNext element * Fix linter with newline * Add Message Migration to Destination Connection Checks (airbytehq#17954) * Add Message Migration to Destination Connection Checks * Fix test setup * Update helm release workflow (airbytehq#18048) * Update workflow * Update trigger rules * fix: Update release workflow with abillity to add tags * Update workflow * Remove unused `airbyte-cli` (airbytehq#18009) * 🐛 [low-code] $options shouldn't overwrite values that are already defined (airbytehq#18060) * fix * Add missing test * remove prints * extract to method * rename * Add missing test * rename * bump * Update helm chart comments (airbytehq#18072) * Update helm charts (airbytehq#18073) * add test * fix chart.yaml * 16250 Destination Redis: Add SSH support (airbytehq#17951) * 16250 Destination Redis: Add SSH support * 16250 Resolve port issue * 11679 Bump version * auto-bump connector version Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com> * Bump helm chart version reference to 0.40.20 (airbytehq#18074) * Bump helm chart version reference to 0.40.20 * remove binary Co-authored-by: xpuska513 <xpuska513@users.noreply.github.com> Co-authored-by: Kyryl Skobylko <xpuska513@gmail.com> * Helm Chart: Create service annotations for airbyte-server (airbytehq#17932) * Support annotations for airbyte-server as well, update version and update docs. * Fix auto-indent. Co-authored-by: Kyryl Skobylko <xpuska513@gmail.com> * Bmoric/remove dep server worker (airbytehq#17894) * test [ci skip] * Autogenerated files * Add missing annotation * Remove unused json2Schema block from worker * Move tess * Missing deps and format * Fix test build * TMP * Add missing dependencies * PR comments * Tmp * [ci skip] Tmp * Fix acceptance test and add the seed dependency * Fix build * For diff * tmp * Build pass * make the worker to be on the platform only * fix setting.yaml * Fix pmd * Fix Cron * Add chart * Fix cron * Fix server build.gradle * Fix jar conflict * PR comments * Add cron micronaut environemnt * Updated connector catalog page (airbytehq#18076) * Move the port forward outside of the main docker-compose (airbytehq#17864) * Bump Airbyte version from 0.40.14 to 0.40.15 (airbytehq#17970) Co-authored-by: benmoriceau <benmoriceau@users.noreply.github.com> * 🎉 Source Shopify: Add metafield streams (airbytehq#17962) * 🎉 Source Shopify: Add metafield streams * Source Shopify: fix unittest * Source Shopify: docs update * Source Shopify: fix backward compatibility test * Source Shopify: fix schemas * Source Shopify: fix state filter * Source Shopify: refactor & optimize * Source Shopify: fix test privileges * Source Shopify: fix stream filter * Source Shopify: fix streams * Source Shopify: update abnormal state * Source Shopify: fix abnormal state streams * Source Shopify: fix streams * updated methods, formated code * Source Shopify: typo fix * auto-bump connector version Co-authored-by: Oleksandr Bazarnov <oleksandr.bazarnov@globallogic.com> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com> * fix check for streams that do not use a stream slicer (airbytehq#18080) * fix check for streams that do not use a stream slicer * increment version and changelog before publish * tolerate database nulls in webhook operation configs (airbytehq#18084) * Implement webhook operation in the sync workflow (airbytehq#18022) Implements the webhook operation as part of the sync workflow. - Introduces the new activity implementation - Updates the various interfaces that pass input to get the relevant configs to the sync workflow - Hooks the new activity into the sync workflow - Passes the webhook configs along into the sync workflow job * Bump helm chart version reference to 0.40.22 (airbytehq#18077) * Added new "filters" python file, along with a "hash" filter. This can… (airbytehq#18000) * Added new "filters" python file, along with a "hash" filter. This can be extended to include other custom filters in the future. * Added additional comments * Moved usage of the hash_obj inside the conditional that confirms it exists * Moved the hash function call inside a condition to ensure that it exists * Fixed the application of the salt , so that it does not modify the hash unless it is actually passed in. * Added unit tests to validate new jinja hash functionality * Updated unit test to pass numeric value as a float instead of string * Removed unreferenced import to pytest * Updated version * format * format * format * format * format Co-authored-by: Alexandre Girard <alexandre@airbyte.io> * Bump helm chart version reference to 0.40.24 (airbytehq#18081) * Bump helm chart version reference to 0.40.24 * Update .gitignore Co-authored-by: benmoriceau <benmoriceau@users.noreply.github.com> Co-authored-by: Kyryl Skobylko <xpuska513@gmail.com> * SATs: allow new records in a sequential read for full refresh test (airbytehq#17660) * SATs: allow new records in a sequential read for full refresh test * SATs: upd changelog * SATs: change the output when failing full refresh test * SATs: upd according to code review * Source facebook-marketing: remove `pixel` from custom conversions stream (airbytehq#18045) * airbytehq#744 source facebook-marketing: rm pixel from custom conversions stream * airbytehq#744 source fb marketing: upd changelog * airbytehq#744 source facebook-marketing - add custom_conversions to the test catalog * auto-bump connector version Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com> * #17506 fix klaviyo & marketo expected_records (airbytehq#18101) Co-authored-by: Edmundo Ruiz Ghanem <168664+edmundito@users.noreply.github.com> Co-authored-by: terencecho <3916587+terencecho@users.noreply.github.com> Co-authored-by: Ryan Fu <ryan.fu@airbyte.io> Co-authored-by: Jimmy Ma <gosusnp@users.noreply.github.com> Co-authored-by: Kyryl Skobylko <xpuska513@gmail.com> Co-authored-by: Evan Tahler <evan@airbyte.io> Co-authored-by: Alexandre Girard <alexandre@airbyte.io> Co-authored-by: Yevhen Sukhomud <suhomud@gmail.com> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: xpuska513 <xpuska513@users.noreply.github.com> Co-authored-by: Prasanth <72515998+sfc-gh-pkommini@users.noreply.github.com> Co-authored-by: Benoit Moriceau <benoit@airbyte.io> Co-authored-by: Amruta Ranade <11484018+Amruta-Ranade@users.noreply.github.com> Co-authored-by: Octavia Squidington III <90398440+octavia-squidington-iii@users.noreply.github.com> Co-authored-by: benmoriceau <benmoriceau@users.noreply.github.com> Co-authored-by: Artem Inzhyyants <36314070+artem1205@users.noreply.github.com> Co-authored-by: Oleksandr Bazarnov <oleksandr.bazarnov@globallogic.com> Co-authored-by: Brian Lai <51336873+brianjlai@users.noreply.github.com> Co-authored-by: Michael Siega <109092231+mfsiega-airbyte@users.noreply.github.com> Co-authored-by: Alexander Marquardt <alexander.marquardt@gmail.com> Co-authored-by: Denys Davydov <davydov.den18@gmail.com>

fix

cffd941

github-actions bot added the CDK Connector Development Kit label Oct 17, 2022

Add missing test

e9e54ce

girarda commented Oct 17, 2022

View reviewed changes

girarda added 4 commits October 17, 2022 08:31

remove prints

d98bc66

extract to method

da275b0

rename

19687bf

Add missing test

e0115d0

girarda changed the title ~~alex/fix_create_partial~~ 🐛 [low-code] $options shouldn't overwrite values that are already defined Oct 17, 2022

rename

10ae205

girarda requested a review from brianjlai October 17, 2022 15:47

girarda marked this pull request as ready for review October 17, 2022 15:47

girarda requested a review from a team as a code owner October 17, 2022 15:47

girarda requested a review from sherifnada October 17, 2022 16:10

sherifnada approved these changes Oct 17, 2022

View reviewed changes

girarda added 2 commits October 17, 2022 10:32

Merge branch 'master' into alex/fix_create_partial

34a6167

bump

13b545d

girarda merged commit 0ffd503 into master Oct 17, 2022

girarda deleted the alex/fix_create_partial branch October 17, 2022 18:07

YatsukBogdan1 pushed a commit that referenced this pull request Oct 18, 2022

🐛 [low-code] $options shouldn't overwrite values that are already def…

01bcb8c

…ined (#18060) * fix * Add missing test * remove prints * extract to method * rename * Add missing test * rename * bump

octavia-squidington-iii mentioned this pull request Oct 20, 2022

Bump Airbyte version from 0.40.15 to 0.40.16 #18268

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 [low-code] $options shouldn't overwrite values that are already defined #18060

🐛 [low-code] $options shouldn't overwrite values that are already defined #18060

girarda commented Oct 17, 2022 •

edited

Loading

girarda Oct 17, 2022

girarda Oct 17, 2022

girarda Oct 17, 2022

sherifnada left a comment

girarda commented Oct 17, 2022 •

edited by github-actions bot

Loading

		@@ -33,6 +33,12 @@ def test_pass_parameter_to_create_function():
		assert object.another_param == "B"


		def test_parameter_not_overwritten_by_options():

🐛 [low-code] $options shouldn't overwrite values that are already defined #18060

🐛 [low-code] $options shouldn't overwrite values that are already defined #18060

Conversation

girarda commented Oct 17, 2022 • edited Loading

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Tests

girarda Oct 17, 2022

Choose a reason for hiding this comment

girarda Oct 17, 2022

Choose a reason for hiding this comment

girarda Oct 17, 2022

Choose a reason for hiding this comment

sherifnada left a comment

Choose a reason for hiding this comment

girarda commented Oct 17, 2022 • edited by github-actions bot Loading

girarda commented Oct 17, 2022 •

edited

Loading

girarda commented Oct 17, 2022 •

edited by github-actions bot

Loading