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

MySQL source resource #486

Merged
merged 11 commits into from
Mar 8, 2024
Merged

MySQL source resource #486

merged 11 commits into from
Mar 8, 2024

Conversation

bobbyiliev
Copy link
Contributor

@bobbyiliev bobbyiliev commented Mar 5, 2024

This is mainly intended to lay the ground work for the MySQL source resource. Probably there are some missing configuration options that I'll have to add here.

@bobbyiliev bobbyiliev marked this pull request as ready for review March 5, 2024 17:49
@bobbyiliev bobbyiliev requested a review from a team as a code owner March 5, 2024 17:49
@bobbyiliev bobbyiliev requested review from arusahni and rjobanp and removed request for a team March 5, 2024 17:49
docs/resources/source_mysql.md Outdated Show resolved Hide resolved
docs/resources/source_mysql.md Outdated Show resolved Hide resolved
return b
}

func (b *SourceMySQLBuilder) Size(s string) *SourceMySQLBuilder {
Copy link

Choose a reason for hiding this comment

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

this isn't relevant anymore, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used by the read func to get the size of the cluster where the source is created in:

COALESCE(mz_sources.size, mz_clusters.size) AS size,

This was part of the migration that allowed us to remove notion of linked clusters in Terraform.

- `ownership_role` (String) The owernship role of the object.
- `region` (String) The region to use for the resource connection. If not set, the default region is used.
- `schema_name` (String) The identifier for the source schema. Defaults to `public`.
- `table` (Block List) Specifies the tables to be included in the source. If not specified, all tables are included. (see [below for nested schema](#nestedblock--table))
Copy link

Choose a reason for hiding this comment

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

should we also add support for the FOR SCHEMAS option?

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 planing to add FOR SCHEMAS option as part of this discussion here which caused some major issues with the Postgres source.

Just reworked an old PR to patch this on the Postgres side, will cherry pick some of the changes here as well.

return diag.FromErr(err)
}
o := materialize.MaterializeObject{ObjectType: "SOURCE", Name: sourceName, SchemaName: schemaName, DatabaseName: databaseName}
// b := materialize.NewSource(metaDb, o)
Copy link

Choose a reason for hiding this comment

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

is this relevant? should we allow updating a source object at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you can update some propertites like the the name of the source, comments, ownership and etc.

I still have to work on implementing the subsources as well, eg. adding and removing subsources ALTER SOURCE "database"."schema"."source" ADD SUBSOURCE "table_1" AS "table_alias";

Copy link

Choose a reason for hiding this comment

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

Ah got it! Just so you know the ALTER SOURCE statements are not yet supported for MySQL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, I've created a follow up issue for this: #490

Copy link

@rjobanp rjobanp left a comment

Choose a reason for hiding this comment

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

nice work!

Copy link

@rjobanp rjobanp left a comment

Choose a reason for hiding this comment

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

test_columns/ignore_columns addition lgtm

@bobbyiliev bobbyiliev merged commit 58ec2e5 into main Mar 8, 2024
6 checks passed
@bobbyiliev bobbyiliev deleted the MySQL-Source-Resource branch March 8, 2024 06:44
@bobbyiliev bobbyiliev mentioned this pull request Mar 8, 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