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

Build datastore index resource. #3085

Merged

Conversation

nat-henderson
Copy link
Contributor

@nat-henderson nat-henderson commented Feb 6, 2020

Add datastore/ product
Add ability for fetching resources from operations
Fixes hashicorp/terraform-provider-google#60

Release Note Template for Downstream PRs (will be copied)

google_datastore_index

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 26 files changed, 763 insertions(+), 47 deletions(-))
Terraform Beta: Diff ( 31 files changed, 794 insertions(+), 56 deletions(-))
TF Conversion: Diff ( 2 files changed, 110 insertions(+))

@nat-henderson
Copy link
Contributor Author

Haven't written examples yet, sending out for early review to make sure I'm headed down the right road. Works in testing on my machine!

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 26 files changed, 763 insertions(+), 47 deletions(-))
Terraform Beta: Diff ( 31 files changed, 794 insertions(+), 56 deletions(-))
TF Conversion: Diff ( 2 files changed, 110 insertions(+))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 26 files changed, 996 insertions(+), 40 deletions(-))
Terraform Beta: Diff ( 30 files changed, 1070 insertions(+), 48 deletions(-))
TF Conversion: Diff ( 2 files changed, 110 insertions(+))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 26 files changed, 996 insertions(+), 40 deletions(-))
Terraform Beta: Diff ( 30 files changed, 1070 insertions(+), 48 deletions(-))
TF Conversion: Diff ( 2 files changed, 110 insertions(+))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 27 files changed, 1108 insertions(+), 40 deletions(-))
Terraform Beta: Diff ( 31 files changed, 1182 insertions(+), 48 deletions(-))
TF Conversion: Diff ( 2 files changed, 110 insertions(+))
TF OiCS: Diff ( 2 files changed, 110 insertions(+))

@nat-henderson
Copy link
Contributor Author

@danawillow Okay, seems to be working for me, ready for review!

products/datastore/api.yaml Outdated Show resolved Hide resolved
products/datastore/api.yaml Outdated Show resolved Hide resolved
products/datastore/api.yaml Outdated Show resolved Hide resolved
products/datastore/api.yaml Outdated Show resolved Hide resolved
products/datastore/terraform.yaml Outdated Show resolved Hide resolved
products/datastore/api.yaml Show resolved Hide resolved
products/datastore/terraform.yaml Show resolved Hide resolved
products/datastore/api.yaml Show resolved Hide resolved
products/datastore/api.yaml Outdated Show resolved Hide resolved
@@ -234,8 +234,39 @@ func resource<%= resource_name -%>Create(d *schema.ResourceData, meta interface{
}
<% end -%>
<% if object.async.is_a? Api::OpAsync -%>
<% if object.async.result.resource_inside_response and not object.identity.empty? -%>
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to check- is the index id not known until the operation is finished, or is it also contained in the first operation that we then poll? I know firestore_index currently solves this with custom code in the post create, because the name is contained in the first operation response that we already have. Looking at the generated output for this we now set the id on that resource 3 times in create, which isn't ideal. Is there a clean solution that would work for that resource and this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right - it's not known until the operation is finished. :( I saw that it could be solved with custom code, but I wanted to make sure that I solved it Once And For All - is there an issue with setting the ID three times? I think we set it once to make sure that we'll do a refresh if we fail in the middle of polling, plus once to get the true final id once all information is known ... and then in firestore, once more due to the custom code solution. I agree it's not clean, and I'm struggling to test it so I don't even know if it works yet (since you can't use the same project for both firestore and datastore).

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. I'm pretty ok with it, then. It shouldn't cause issues for users and especially with these templates going away at some point, I don't think it's worth spending a ton of time finding the most-clean-code way to solve this problem for two resources with similar but not identical needs.

@nat-henderson
Copy link
Contributor Author

Resolved all comments but one - the uncleanliness of this solution when combined with Firestore's solution for a similar, but different, problem. I can try to find something, but at first glance it's not obvious how to avoid it, other than making this operation change into custom code. I resolve that tradeoff in this direction, but I'm willing to change it - is it worth it, in your opinion?

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 26 files changed, 994 insertions(+), 40 deletions(-))
Terraform Beta: Diff ( 30 files changed, 1068 insertions(+), 48 deletions(-))
TF Conversion: Diff ( 2 files changed, 110 insertions(+))
TF OiCS: Diff ( 2 files changed, 110 insertions(+))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 27 files changed, 1003 insertions(+), 40 deletions(-))
Terraform Beta: Diff ( 31 files changed, 1077 insertions(+), 48 deletions(-))
TF Conversion: Diff ( 2 files changed, 110 insertions(+))
TF OiCS: Diff ( 2 files changed, 110 insertions(+))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 27 files changed, 1003 insertions(+), 40 deletions(-))
Terraform Beta: Diff ( 31 files changed, 1077 insertions(+), 48 deletions(-))
TF Conversion: Diff ( 2 files changed, 110 insertions(+))
TF OiCS: Diff ( 2 files changed, 110 insertions(+))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 27 files changed, 1012 insertions(+), 40 deletions(-))
Terraform Beta: Diff ( 31 files changed, 1088 insertions(+), 48 deletions(-))
TF Conversion: Diff ( 2 files changed, 110 insertions(+))
TF OiCS: Diff ( 2 files changed, 110 insertions(+))

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 27 files changed, 1012 insertions(+), 40 deletions(-))
Terraform Beta: Diff ( 31 files changed, 1088 insertions(+), 48 deletions(-))
TF Conversion: Diff ( 2 files changed, 110 insertions(+))
TF OiCS: Diff ( 2 files changed, 110 insertions(+))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Google DataStore indexes resource
4 participants