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

Remove required table field for share point connector #354

Conversation

fivetran-jovanmanojlovic
Copy link
Contributor

No description provided.

@fivetran-jovanmanojlovic fivetran-jovanmanojlovic marked this pull request as ready for review September 3, 2024 11:39
@fivetran-jovanmanojlovic fivetran-jovanmanojlovic force-pushed the Remove_required_table_field_for_share_point_connector branch from da96a3b to d4ae7f7 Compare September 3, 2024 11:53
@@ -659,7 +659,8 @@
"key_field": "",
"item_type": {},
"description": {
"adobe_analytics": "The table name unique within the schema to which connector will sync the data. Required for connector creation."
"adobe_analytics": "The table name unique within the schema to which connector will sync the data. Required for connector creation.",
"share_point": "The table name within the schema to which the connector will sync the data."
Copy link
Collaborator

Choose a reason for hiding this comment

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

how come we have it here? it's totally unrelated field for share_point.

@@ -10508,7 +10509,6 @@
"kinesis": "Destination table. Table is permanent and cannot be changed after connection creation",
"s3": "Destination table. Table is permanent and cannot be changed after connection creation",
"sftp": "Destination table. Table is permanent and cannot be changed after connection creation",
"share_point": "Destination table. Table is permanent and cannot be changed after connection creation",
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be still here, but we should handle it as optional field for share_point.

Copy link
Collaborator

@beevital beevital left a comment

Choose a reason for hiding this comment

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

LGTM, this is the correct solution so should work. Please check my minor suggestion below.

@@ -328,10 +328,9 @@ func getDestinatonSchemaForConfig(serviceId, nameAttr, tableAttr, prefixAttr att
"schema": nameAttr.(types.String).ValueString(),
}
if common.GetDestinationSchemaFields()[service]["table"] {
if tableAttr.IsNull() {
return nil, fmt.Errorf("`destination_schema.table` field is required to create `%v` connector", service)
if !tableAttr.IsNull() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We also should check if it is not empty string, just in case.

@fivetran-jovanmanojlovic fivetran-jovanmanojlovic merged commit 562a545 into main Sep 5, 2024
1 check passed
@fivetran-jovanmanojlovic fivetran-jovanmanojlovic deleted the Remove_required_table_field_for_share_point_connector branch September 5, 2024 09:18
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