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

✏️ fixing the address_spaces schema property for vNET #3384

Conversation

bgauduch
Copy link

@bgauduch bgauduch commented May 6, 2019

PR regarding issue #3373

Provider compiled & tested with modifications (both vNET resource and datasource).
The doc d/virtual_network.html.markdown seem's to be already up-to-date.

Any feedbacks and reviews greatly appreciated 😄

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Hi @bgauduch,

Thanks for spotting this. Unfortunately changing this is a breaking change and required a bit more thought then just updating the schema (as well as it should probably be address_prefixes). We need to add the new property, mark both as computed and conflicted, mark the old one as deprecated. On update use the new one and on read set both.

Given that is a bit larger in scope for now we could update the docs and then add another property address_space to the datasource so they match for now. WDYT?

@bgauduch
Copy link
Author

bgauduch commented May 7, 2019

Hi @katbyte,

Thanks for your review and feedback !

I was wondering if you have any process for this kind of change afterward opening this PR, well now I know 👌
By the way, some kind of contributing guidelines with this repository workflow would definitely be a plus for newcomers like me 😉

I agree with you: we should make the doc match the actual behavior first as a quick fix, but in the end the attribute should be updated.
I will revert my changes on resource_arm_virtual_network.go and update the documentation as well as this PR title for now.

But some things do confuse me in your feedback and in the azure documentation / usage of vNET in general:

  • I'm not sure which property name should be used. address_prefixes is referenced both in the azure doc on vNET and in the az CLI, but address_spaces is used in the API doc. I believe Terraform Providers should stick to the API, WDYT ?
  • It looks like the API allow only one address space per vNET according to the API doc, but one can in fact bind multiple spaces to one vNET using the az CLI... really confusing.
  • Beside, address_prefixes seems to be related mostly to azure subnets, which do supports multiple prefixes according to the API (which make sense).

Anyway, it looks like the Azure documentation on vNET (API, CLI, PWSH, web, etc.) is not quite up-to-date / homogeneous, I might take some time opening issues / PR on it.

Regards,
Baptiste

@ghost ghost removed the waiting-response label May 7, 2019
@bgauduch
Copy link
Author

bgauduch commented May 7, 2019

Well, the documentation is already up-to-date with the current attribute naming (address_space in everything related to vNET, and address_spaces in everything related to vNET datasource). 👍

So this PR can be closed, I will add a summary in the issue.

Regards,
Baptiste

@ghost
Copy link

ghost commented Jun 21, 2019

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 Jun 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants