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

destination-mysql: convert prod code to kotlin #40553

Conversation

stephane-airbyte
Copy link
Contributor

What

How

Review guide

User Impact

Can this PR be safely reverted and rolled back?

  • YES πŸ’š
  • NO ❌

Copy link

vercel bot commented Jun 26, 2024

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Comments Updated (UTC)
airbyte-docs βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Jun 26, 2024 7:05pm

Copy link
Contributor

@edgao edgao left a comment

Choose a reason for hiding this comment

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

I didn't look at the actual java<>kotlin conversion, but had some questions about the other files

@@ -11,6 +11,7 @@
import io.airbyte.cdk.integrations.base.spec_modification.SpecModifyingDestination;
import io.airbyte.commons.json.Jsons;
import io.airbyte.protocol.models.v0.ConnectorSpecification;
import io.airbyte.integrations.destination.mysql.MySQLDestination;
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these new imports necessary?

@@ -11,6 +11,6 @@ import org.testcontainers.utility.DockerImageName
/** Much like the destination-postgres PostgresTestDatabase, this was copied from source-mysql. */
class MySQLContainerFactory : ContainerFactory<MySQLContainer<*>>() {
override fun createNewContainer(imageName: DockerImageName?): MySQLContainer<*> {
return MySQLContainer(imageName?.asCompatibleSubstituteFor("mysql"))
return MySQLContainer(imageName?.asCompatibleSubstituteFor("io/airbyte/integrations/destination/mysql"))
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this? afaict mysql is meant to be a docker image name, not a java package name?

Copy link
Contributor Author

@stephane-airbyte stephane-airbyte Jun 26, 2024

Choose a reason for hiding this comment

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

some stupid refactoring coming from me moving files into the kotlin directory... Thank you for noticing it

@stephane-airbyte stephane-airbyte force-pushed the stephane/06-26-destination-mysql_convert_prod_code_to_kotlin branch from c396f63 to 17caebf Compare June 26, 2024 17:26
Copy link
Contributor Author

stephane-airbyte commented Jun 26, 2024

@stephane-airbyte stephane-airbyte force-pushed the stephane/06-26-destination-mysql_convert_prod_code_to_kotlin branch 2 times, most recently from 8ba9753 to 5a961e1 Compare June 26, 2024 17:30
@stephane-airbyte stephane-airbyte force-pushed the stephane/06-26-destination-mysql_convert_prod_code_to_kotlin branch from 5a961e1 to 4d53da7 Compare June 26, 2024 17:34
Copy link
Contributor Author

stephane-airbyte commented Jun 27, 2024

Merge activity

@stephane-airbyte stephane-airbyte merged commit 4b17639 into master Jun 27, 2024
32 checks passed
@stephane-airbyte stephane-airbyte deleted the stephane/06-26-destination-mysql_convert_prod_code_to_kotlin branch June 27, 2024 20:45
xiaohansong pushed a commit that referenced this pull request Jul 9, 2024
## What
<!--
* Describe what the change is solving. Link all GitHub issues related to this change.
-->

## How
<!--
* Describe how code changes achieve the solution.
-->

## Review guide
<!--
1. `x.py`
2. `y.py`
-->

## User Impact
<!--
* What is the end result perceived by the user?
* If there are negative side effects, please list them. 
-->

## Can this PR be safely reverted and rolled back?
<!--
* If unsure, leave it blank.
-->
- [ ] YES πŸ’š
- [ ] NO ❌
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants