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

🐛 Source Dynamodb: Fix reserved words in expression #20172

Merged
merged 30 commits into from
Feb 27, 2023
Merged

🐛 Source Dynamodb: Fix reserved words in expression #20172

merged 30 commits into from
Feb 27, 2023

Conversation

itaseskii
Copy link
Contributor

@itaseskii itaseskii commented Dec 7, 2022

What

Fixes #19739

How

Using placeholders for attributes in the filter expression.

Recommended reading order

  1. x.java
  2. y.python

🚨 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

  • Community member? 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
    • docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
    • docs/integrations/README.md
    • airbyte-integrations/builds.md
  • 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 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.

@itaseskii itaseskii changed the title 🐛 Source Dynamodb: Fix reserved words in expressions 🐛 Source Dynamodb: Fix reserved words in expression Dec 7, 2022
@itaseskii
Copy link
Contributor Author

itaseskii commented Dec 13, 2022

/test connector=connectors/source-dynamodb

🕑 connectors/source-dynamodb https://github.com/airbytehq/airbyte/actions/runs/3688353203
❌ connectors/source-dynamodb https://github.com/airbytehq/airbyte/actions/runs/3688353203
🐛

@itaseskii
Copy link
Contributor Author

itaseskii commented Dec 13, 2022

/test connector=connectors/source-dynamodb

🕑 connectors/source-dynamodb https://github.com/airbytehq/airbyte/actions/runs/3689956136
✅ connectors/source-dynamodb https://github.com/airbytehq/airbyte/actions/runs/3689956136
No Python unittests run

Build Passed

Test summary info:

All Passed

@vincentkoc vincentkoc requested review from vincentkoc and a team December 15, 2022 09:52
Copy link
Contributor

@vincentkoc vincentkoc left a comment

Choose a reason for hiding this comment

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

Missing the docker version bump and notes

@vincentkoc
Copy link
Contributor

@itaseskii can you please update the link in the PR to the exact issue that this is resolving for full context of the change?

@piyushsingariya
Copy link

@itaseskii can you please update the link in the PR to the exact issue that this is resolving for full context of the change?

@koconder the PR has been linked to the issue already, could you re-review the PR again?

@vincentkoc vincentkoc self-assigned this Jan 15, 2023
vincentkoc
vincentkoc previously approved these changes Jan 19, 2023
@octavia-squidington-iv octavia-squidington-iv added the area/documentation Improvements or additions to documentation label Jan 19, 2023
@vincentkoc
Copy link
Contributor

vincentkoc commented Jan 19, 2023

/publish connector=connectors/source-dynamodb

🕑 Publishing the following connectors:
connectors/source-dynamodb
https://github.com/airbytehq/airbyte/actions/runs/3957461279


Connector Did it publish? Were definitions generated?
connectors/source-dynamodb

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@vincentkoc
Copy link
Contributor

@itaseskii see publish image error logs, something is not right. Its not liking the code changes for some reason.

@sajarin sajarin added bounty bounty-S Maintainer program: claimable small bounty PR labels Jan 19, 2023
@itaseskii
Copy link
Contributor Author

itaseskii commented Jan 19, 2023

/test connector=connectors/source-dynamodb

🕑 connectors/source-dynamodb https://github.com/airbytehq/airbyte/actions/runs/3962228595
✅ connectors/source-dynamodb https://github.com/airbytehq/airbyte/actions/runs/3962228595
No Python unittests run

Build Passed

Test summary info:

All Passed

@itaseskii
Copy link
Contributor Author

@itaseskii see publish image error logs, something is not right. Its not liking the code changes for some reason.

@koconder there were some changes in the protocol model which made the connector incompatible. the issue should be fixed now, please try again.

@sh4sh
Copy link
Contributor

sh4sh commented Feb 23, 2023

/publish connector=connectors/source-dynamodb

🕑 Publishing the following connectors:
connectors/source-dynamodb
https://github.com/airbytehq/airbyte/actions/runs/4253307965


Connector Did it publish? Were definitions generated?
connectors/source-dynamodb

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@sh4sh
Copy link
Contributor

sh4sh commented Feb 23, 2023

Published but checks failing now... hang on

@sh4sh sh4sh enabled auto-merge (squash) February 23, 2023 21:58
@sh4sh
Copy link
Contributor

sh4sh commented Feb 23, 2023

Something is amiss with connectors.md that is causing the build to fail. I'll take a look tomorrow morning, for now I have rolled back the above /publish.

Update 2/24: I wasn't able to resolve this today, but I will come back to it on Monday

@sh4sh sh4sh merged commit b9b8cb0 into airbytehq:master Feb 27, 2023
@sh4sh
Copy link
Contributor

sh4sh commented Feb 27, 2023

The connectors.md issue was resolved, but now getting GitHub self-hosted runner registration error on the Connectors Base: Start Build EC2 Runner step which i think is a transient error. Fortunately the connectors base build passed before so I think we are probably in the clear. Re-running here: https://github.com/airbytehq/airbyte/actions/runs/4286881888

Oops, we still need to publish this too as that got rolled back. I should have disabled auto-merge. Reverting now.

@sh4sh
Copy link
Contributor

sh4sh commented Mar 2, 2023

/publish connector=connectors/source-dynamodb

🕑 Publishing the following connectors:
connectors/source-dynamodb
https://github.com/airbytehq/airbyte/actions/runs/4316142995


Connector Did it publish? Were definitions generated?
connectors/source-dynamodb

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@sh4sh
Copy link
Contributor

sh4sh commented Mar 2, 2023

Please ignore the above message ^ these changes have been merged into source-dynamodb:0.1.2

@murat-cetinkaya
Copy link

Thanks @sh4sh 👏🏼

danielduckworth pushed a commit to danielduckworth/airbyte that referenced this pull request Mar 13, 2023
* fix reserved words in expression

* chore: bump version

* fix state message

* auto-bump connector version

* fix reserved words in projection expression

* bump dockerfile version

* auto-bump connector version

---------

Co-authored-by: Vincent Koc <vincentkoc@ieee.org>
Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
Co-authored-by: Sajarin <sajarindider@gmail.com>
Co-authored-by: Sunny <6833405+sh4sh@users.noreply.github.com>
danielduckworth pushed a commit to danielduckworth/airbyte that referenced this pull request Mar 13, 2023
danielduckworth pushed a commit to danielduckworth/airbyte that referenced this pull request Mar 13, 2023
@murat-cetinkaya
Copy link

I upgraded to v0.42 which includes this fix but I'm still getting the same error (DynamoDbException: Invalid ProjectionExpression: Syntax error; token: "-", ).
I wonder about your experiences.

@itaseskii
Copy link
Contributor Author

@murat-cetinkaya
Copy link

Oh, I overlooked that. I tried again and it succeeded 🎉. Thanks again @itaseskii 👏🏼👏🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation bounty bounty-S Maintainer program: claimable small bounty PR connectors/source/dynamodb
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source DynamoDB - Reserved Keyword Issue