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

Move Bigtable config to cluster block. #2161

Merged
merged 2 commits into from
Oct 4, 2018
Merged

Conversation

paddycarver
Copy link
Contributor

Match the bigtable_instance resource to the API structure, deprecating
the inlined config parameters. This is a complicated deprecation,
because we don't want to ForceNew when updating to use the new
undeprecated fields, but we want to ForceNew if they change.

This is achieved using a CustomizeDiff.

Match the bigtable_instance resource to the API structure, deprecating
the inlined config parameters. This is a complicated deprecation,
because we don't want to ForceNew when updating to use the new
undeprecated fields, but we want to ForceNew if they change.

This is achieved using a CustomizeDiff.
@paddycarver paddycarver added this to the 1.19.0 milestone Oct 3, 2018
@ghost ghost added the size/xl label Oct 3, 2018
Copy link
Collaborator

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

Can you add a test with multiple clusters?

Does using a default zone make sense in the multiple-cluster world? Can we make a Cloud Bigtable with 2 clusters in the same zone? I don't think that makes sense and they should be in multiple zones, so the API probably won't allow it.

@paddycarver
Copy link
Contributor Author

Can you add a test with multiple clusters?

Does using a default zone make sense in the multiple-cluster world? Can we make a Cloud Bigtable with 2 clusters in the same zone? I don't think that makes sense and they should be in multiple zones, so the API probably won't allow it.

I have no idea. Since this wasn't supposed to be enhancement work, just deprecation, how would you feel about me just setting MaxItems to 1 for now, and we can sort through the enhancements at a later date? At this point, I just want to get the config in the right shape, I don't really care about supporting multiple clusters today. We could do that work some other time.

Copy link
Collaborator

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

SGTM. We should have that done for 2.0.0 though probably and not just "some other time"? Or removing the default zone done then at least, as it's a breaking change.

@paddycarver
Copy link
Contributor Author

I'd be down for removing getZone in 2.0.0, but we can't do that today.

@rileykarson
Copy link
Collaborator

Oh yup! That's what I meant by that*

Only allow one cluster for bigtable instances. We can expand to 2 for
2.0.0.
@paddycarver
Copy link
Contributor Author

Cool, I already have #1672 in the 2.0.0 milestone, so we've got an issue to track that enhancement.

@paddycarver paddycarver merged commit d6cf446 into master Oct 4, 2018
@paultyng paultyng deleted the paddy_bigtable_clusters branch October 29, 2018 19:27
chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018
…able_clusters

Move Bigtable config to cluster block.
@ghost
Copy link

ghost commented Nov 16, 2018

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Nov 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants