-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Support custom schema in Healthcare HL7 V2 stores #3261
Conversation
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform Beta: Diff ( 2 files changed, 69 insertions(+), 13 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform Beta: Diff ( 2 files changed, 74 insertions(+), 13 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform Beta: Diff ( 2 files changed, 74 insertions(+), 13 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform Beta: Diff ( 2 files changed, 433 insertions(+), 333 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform Beta: Diff ( 2 files changed, 78 insertions(+), 13 deletions(-)) |
@danawillow how does this look? I took all your suggestions except handling the error for json.Marshal call in flatten in other places. Given gcp rest APIs always return valid JSON anyway I think we could assume for the time being that the API will return a valid JSON value which is marshallable, until the templates are updated to return error in flatten. Is that acceptable? Also, for some reason skip_tests were set on all the examples, I am not sure why but I imagine having tests for this would be nice. NOTE: the current schema in the example is empty, I am working on a minimal schema to add here. Will update back when I get it. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform Beta: Diff ( 2 files changed, 78 insertions(+), 13 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform Beta: Diff ( 2 files changed, 78 insertions(+), 13 deletions(-)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks reasonable to me!
templates/terraform/custom_expand/healthcare_hl7_v2_store_schema.erb
Outdated
Show resolved
Hide resolved
templates/terraform/custom_flatten/healthcare_hl7_v2_store_schema.erb
Outdated
Show resolved
Hide resolved
templates/terraform/examples/healthcare_hl7_v2_store_parser_config.tf.erb
Show resolved
Hide resolved
templates/terraform/custom_flatten/healthcare_hl7_v2_store_schema.erb
Outdated
Show resolved
Hide resolved
…ma.erb Co-Authored-By: Dana Hoffman <danahoffman@google.com>
Co-Authored-By: Dana Hoffman <danahoffman@google.com>
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform Beta: Diff ( 2 files changed, 78 insertions(+), 13 deletions(-)) |
2 similar comments
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform Beta: Diff ( 2 files changed, 78 insertions(+), 13 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform Beta: Diff ( 2 files changed, 78 insertions(+), 13 deletions(-)) |
Done. PTAL (note tests won't pass as this field is still not in beta. ETA ~3 weeks). |
Seems fine. I'm going to hold off on marking it as approved until we can actually run tests. In the meantime, it would be worth confirming that the provider builds locally with these changes, since our cloud build run failed. |
@danawillow the feature has been added in beta and I verified it builds locally. Can you check what error is reported by Cloud Build? |
|
@danawillow success! Bonus I stopped skipping the basic test too. |
I'm seeing two test failures:
|
Thanks Dana. Maybe because I was out of sync with master? I have synced now and it's showing 3 successful checks, so I am hoping it's OK now. |
The status checks are for basic things like whether the generated code compiles. We don't run acceptance tests automatically because they create real GCP resources. https://github.com/GoogleCloudPlatform/magic-modules#testing-your-changes talks a bit about generating the code and running tests on it yourself, but let me know if you have questions or want to walk through running and debugging them and I can help. I'll let you see if you can figure it out first though :) |
Fixed.
|
templates/terraform/examples/healthcare_hl7_v2_store_parser_config.tf.erb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
…m#3261) * add schema field and custom expander * rename * fix custom expand file ref * add custom flatten * actually add the files * rm extra file * add state and validate funcs * single line * shorter var * don't return error on flatten * Update templates/terraform/custom_expand/healthcare_hl7_v2_store_schema.erb Co-Authored-By: Dana Hoffman <danahoffman@google.com> * Apply suggestions from code review Co-Authored-By: Dana Hoffman <danahoffman@google.com> * enable test for parser config * add schema eample * rm dup func, enable basic hl7 test * min version beta * fix examples * fix flatten * heredoc Co-authored-by: Dana Hoffman <danahoffman@google.com>
Replace hashicorp/terraform-provider-google-beta#1858
NOTE: This field is still in alpha and will be launched in the coming weeks, the goal of this PR is to get any reviews out of the way so this can be merged in without additional delays.
Release Note Template for Downstream PRs (will be copied)