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.

Sign up for GitHub

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

Jump to bottom

Add support for Heroku Private Space CIDR and Data CIDR #167

Merged
merged 4 commits into from
Feb 1, 2019
Merged

Add support for Heroku Private Space CIDR and Data CIDR #167

merged 4 commits into from
Feb 1, 2019

Conversation

shawncatz
Copy link
Contributor

@shawncatz shawncatz commented Jan 29, 2019 •
edited
Loading

  • Add cidr and data_cidr to heroku_space resource

This is my first time contributing code to a terraform project, so definitely open to feedback on code style and such.

I've had a lot of trouble getting the acceptance tests to run, but it appears things are working.

@shawncatz
Copy link
Contributor Author

I've been able to test this today using the built provider in ~/.terraform.d/plugins and everything is working fine so far.

davidji99
davidji99 reviewed Jan 30, 2019
View reviewed changes
heroku/resource_heroku_space.go Outdated
@@ -29,6 +29,20 @@ func resourceHerokuSpace() *schema.Resource {
},

Schema: map[string]*schema.Schema{
"cidr": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we move L32-45 after the organization definition? This would better align with heroku's reference of required params first, followed by optional params.
screen shot 2019-01-30 at 10 40 11

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, happy to move them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one note here: I put these in alphabetical order, because it seemed the others were as well. If you prefer me to still move them, let me know.

davidji99
davidji99 reviewed Jan 30, 2019
View reviewed changes
heroku/resource_heroku_space.go Outdated
"data_cidr": {
Type: schema.TypeString,
Optional: true,
Default: "10.1.0.0/16",
Copy link
Collaborator

@davidji99 davidji99 Jan 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not seeing a default value set in the docs. May I ask where you're getting "10.1.0.0/16" from?

EDIT: Ah, I see. That's the default value when I GET the space. Interesting that it doesn't list in the API docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is an undocumented default. I've created spaces without specifying the values for cidr or data_cidr and the data_cidr is set to 10.1.0.0./16

I'd actually prefer to leave the defaults out of here, but I couldn't figure out how to get the value to be removed from the JSON POST data (they would always result in empty strings). If you can point me in the direction of solving that way, I would remove the defaults from the terraform code.

davidji99
davidji99 reviewed Jan 30, 2019
View reviewed changes
heroku/data_source_heroku_space.go Outdated
@@ -8,6 +8,16 @@ func dataSourceHerokuSpace() *schema.Resource {
return &schema.Resource{
Read: dataSourceHerokuSpaceRead,
Schema: map[string]*schema.Schema{
"cidr": {
Copy link
Collaborator

@davidji99 davidji99 Jan 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

You probably want to add the Computed declaration for cidr and data_cidr.

@davidji99
Copy link
Collaborator

Hey @shawncatz,

Thanks for the PR! Two initial questions:

  1. Any particular reason for updating the vendored heroku-go in this PR? Did you need something on heroku-go's master for your change? I ask because this drastically increases your PRs blast radius so it might be good to separate these changes into different PRs if not needed.
  2. You'll also need to update the docs for the data source and the resource.

Thanks!

@shawncatz
Copy link
Contributor Author

shawncatz commented Jan 30, 2019 •
edited
Loading

  1. I pulled in the changes to heroku-go because the vendored version didn't support the cidr and data_cidr options, so I just grabbed the current master. I'm not against splitting up the changes into separate PR's if you would prefer that.
  2. apologies. total fail. will get those updated.

@ghost ghost added the documentation label Jan 30, 2019
@shawncatz shawncatz force-pushed the support-space-cidrs branch from d81014d to 909e656 Compare January 30, 2019 19:07
@shawncatz
Copy link
Contributor Author

I believe I have addressed the PR comments. Let me know if there's anything else.

@talbright talbright mentioned this pull request Jan 30, 2019
Closed
@mars mars self-requested a review January 31, 2019 00:08
mars
mars requested changes Jan 31, 2019
View reviewed changes
Copy link
Member

@mars mars left a comment

Choose a reason for hiding this comment

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

Thank you for this @shawncatz 🙇‍♂️😄

I agree @davidji99 that the vendored heroku-go client should be a separate PR.

We have other folks wanting the freshest heroku-go client, so now is the time ✨

@shawncatz shawncatz mentioned this pull request Jan 31, 2019
Merged
@shawncatz
Copy link
Contributor Author

once #169 is completed / merged I will update this PR

@mars mars changed the title Add support for Heroku Private Space CIDR and Data CIDR, and update heroku-go Add support for Heroku Private Space CIDR and Data CIDR Jan 31, 2019
@mars
Copy link
Member

mars commented Jan 31, 2019

@shawncatz #169 has been merged. Go ahead at your convenience!

@ghost ghost added size/M and removed size/XXL labels Jan 31, 2019
@shawncatz
Copy link
Contributor Author

Thanks @mars! 👍
this is updated to only include the changes for cidr and data_cidr support.

davidji99
davidji99 approved these changes Feb 1, 2019
View reviewed changes
Copy link
Collaborator

@davidji99 davidji99 left a comment

Choose a reason for hiding this comment

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

👍

@davidji99
Copy link
Collaborator

davidji99 commented Feb 1, 2019 •
edited
Loading

@mars you have any further comments? If not, I'd like to merge this and get this in v1.7.4.

EDIT: Actually, we should probably bump to v1.7.5/

mars
mars approved these changes Feb 1, 2019
View reviewed changes
Copy link
Member

@mars mars left a comment

Choose a reason for hiding this comment

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

Yes, looks great 👍

@bcwilsondotcom
Copy link

Just a friendly bump. I’d love if we could get this merged. It’s extremely relevant for me as I’m migrating my company’s heroku infrastructure to private spaces this week.

@mars
Copy link
Member

mars commented Feb 1, 2019

I'll merge, prep the CHANGELOG, and request a release from Hashicorp 🏁

@mars mars merged commit f702594 into heroku:master Feb 1, 2019
@shawncatz shawncatz deleted the support-space-cidrs branch February 1, 2019 18:19
@shawncatz
Copy link
Contributor Author

WOOT! Thanks all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mars mars mars approved these changes

@davidji99 davidji99 davidji99 approved these changes

Assignees
No one assigned
Labels
dependencies documentation size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shawncatz @davidji99 @mars @bcwilsondotcom