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

Expose additional fields in resource_sql_source_representation_instance #3724

Closed
wants to merge 3 commits into from
Closed

Expose additional fields in resource_sql_source_representation_instance #3724

wants to merge 3 commits into from

Conversation

rsinnet
Copy link

@rsinnet rsinnet commented Oct 12, 2021

Expose additional fields from the v1beta4 API:

Add the following members:

  • username
  • password
  • caCertificate
  • clientCertificate
  • clientKey
  • dumpFilePath

See also: https://cloud.google.com/sql/docs/mysql/admin-api/rest/v1beta4/instances#onpremisesconfiguration

@rsinnet
Copy link
Author

rsinnet commented Oct 12, 2021

Judging by the API in google_sql_database_instance, it may not be necessary to use source representations for replication? I suspect it can be done with a single resource. It wasn't apparent from the source code so not really sure but going to experiment. All the necessary fields to populate onPremisesConnection are present, so perhaps this API is not fully documented or more likely I don't know how to read the documentation 😆

https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/sql_database_instance

@rsinnet
Copy link
Author

rsinnet commented Oct 14, 2021

@megan07 I saw your comment about MagicModules here: #3596 (review)

If I were to reimplement this upstream, would this PR be valuable? Unfortunately I did this before looking through the upstream repo, but looks like this just requires updating the appropriate api.yaml with no overrides (maybe validation on the dumpFilePath if there is a similar instance of validation in the codebase), and then add/update integration tests.

Thoughts?

@megan07
Copy link
Contributor

megan07 commented Oct 15, 2021

Hi @rsinnet! If it would provide value to you then I would say it's valuable! And yes, that should be all you'd need to do is to update the appropriate api.yaml. As for overrides, I can't speak to that as I'm not super familiar with the resource, but of course if you run into issues while implementing you can add the PR and we can respond to any questions/concerns. We'd also appreciate if you added a test with the added fields as well. Let me know if you have further questions and I can try to help out!

Thanks for contributing!

@rsinnet rsinnet closed this Nov 17, 2021
@rsinnet rsinnet deleted the user/rsinnet/add-sql-source-representation-instance-properties branch November 17, 2021 14:15
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