Skip to content
This repository has been archived by the owner on Aug 16, 2022. It is now read-only.

Ensure maximum table name length #356

Merged
merged 7 commits into from
Dec 20, 2021
Merged

Ensure maximum table name length #356

merged 7 commits into from
Dec 20, 2021

Conversation

disq
Copy link
Member

@disq disq commented Dec 17, 2021

could add it in cq-provider-sdk/provider/testing instead maybe...

@disq disq requested a review from roneli December 17, 2021 12:57
@disq disq requested a review from irmatov December 17, 2021 15:16
@roneli
Copy link
Contributor

roneli commented Dec 17, 2021

Would suggest to add these checks in SDK will open a referencing issue, or at least verify why it didn't work there.

cc @disq

@disq
Copy link
Member Author

disq commented Dec 17, 2021

@roneli not sure how to do that and keep it sane (it's a test but hang on it's not a test that's meant to run on the SDK itself... gets a provider and generates a test fn?)

Another issue, since the renamed column image_config_repository_auth_config_repository_credentials_provider_arn didn't exist (can't exist unless people have custom-compiled postgres with higher-limit identifiers), is the migration really needed?

@disq
Copy link
Member Author

disq commented Dec 17, 2021

we could attempt to have a smarter the migration and try to alter column rename image_config_repository_auth_config_repository_credentials_prov (the cut-out column name to 63 chars) to the target column maybe?

@roneli
Copy link
Contributor

roneli commented Dec 17, 2021

we could attempt to have a smarter the migration and try to alter column rename image_config_repository_auth_config_repository_credentials_prov (the cut-out column name to 63 chars) to the target column maybe?

You can try to rename on both if one exists it will just work

@roneli
Copy link
Contributor

roneli commented Dec 17, 2021

@roneli not sure how to do that and keep it sane (it's a test but hang on it's not a test that's meant to run on the SDK itself... gets a provider and generates a test fn?)

Another issue, since the renamed column image_config_repository_auth_config_repository_credentials_provider_arn didn't exist (can't exist unless people have custom-compiled postgres with higher-limit identifiers), is the migration really needed?

The SDK has a feature on provider serve it will fail if table added in resource map are in valid, it does the check on runtime, there might be a bug there.

@disq
Copy link
Member Author

disq commented Dec 17, 2021

we could attempt to have a smarter the migration and try to alter column rename image_config_repository_auth_config_repository_credentials_prov (the cut-out column name to 63 chars) to the target column maybe?

You can try to rename on both if one exists it will just work

apparently no need as it will truncate the identifier in the ALTER as well:
NOTICE: identifier "image_config_repository_auth_config_repository_credentials_provider_arn" will be truncated to "image_config_repository_auth_config_repository_credentials_prov"

@disq
Copy link
Member Author

disq commented Dec 18, 2021

SDK will need one more upgrade

@disq
Copy link
Member Author

disq commented Dec 20, 2021

Ready to review/merge @roneli

@roneli roneli merged commit e612af2 into main Dec 20, 2021
@roneli roneli deleted the feat/test-table-name-length branch December 20, 2021 11:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants