-
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
FHIR store streaming config #3611
FHIR store streaming config #3611
Conversation
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 443 insertions(+)) |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=118754" |
See https://github.com/FHIR/sql-on-fhir/blob/master/sql-on-fhir.md. | ||
default_value: :ANALYTICS | ||
values: | ||
- :ANALYTICS |
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.
Only one value available as of now, but it looks like at least one more will be added eventually. The beta version of the api allows you to specify another value, but doing so returns an error stating it isn't supported.
Ran my generated test file in RECORDING in the VCR job |
required: true | ||
description: | | ||
The configuration for the exported BigQuery schema. | ||
properties: |
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.
Would it make sense to mark one of these properties as required
(probably schemaType?)? Just to prevent the empty block situation here: https://github.com/terraform-providers/terraform-provider-google/wiki/Developer-Best-Practices#default-values
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.
What is your opinion on schemaType
? I thought about just removing it since there was only one valid option, and it feels weird requiring it. I agree we should avoid the empty block however
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.
Well if we remove it then we have to make the other field required, right?
I guess if we end up marking the recursive field as required then we could keep this one around with a default value
I don't have strong opinions either way
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.
Yep, making recursiveStructureDepth
required seems fine, I'll do that.
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 443 insertions(+)) |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=118782" |
Fixes hashicorp/terraform-provider-google#6151
Release Note Template for Downstream PRs (will be copied)