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

New Resource: Azure Network Profile #2636

Merged
merged 12 commits into from
May 14, 2019
Merged

New Resource: Azure Network Profile #2636

merged 12 commits into from
May 14, 2019

Conversation

metacpp
Copy link
Contributor

@metacpp metacpp commented Jan 10, 2019

This PR added the new resource: azurerm_network_profile, which is the dependent resource to support virtual network for Azure Container Group requested in #2314.

@metacpp metacpp changed the title NetworkProfile resource [New Resource] azurerm_network_profile Jan 10, 2019
@metacpp metacpp changed the title [New Resource] azurerm_network_profile New Resource: Azure Network Profile Jan 10, 2019
@metacpp metacpp added this to the 2.0.0 milestone Jan 10, 2019
@ghost ghost added the documentation label Jan 10, 2019
@tombuildsstuff tombuildsstuff removed this from the 2.0.0 milestone Jan 10, 2019
@metacpp metacpp modified the milestone: 2.0.0 Jan 10, 2019
@metacpp
Copy link
Contributor Author

metacpp commented Jan 11, 2019

Just ran the tests and they all passed:
image

@bentterp
Copy link
Contributor

@tombuildsstuff I really hope you can find time to review this. If we can run containers inside vnets,m our security would be much improved.
@metacpp Thank you very much indeed!

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.

thank you for the PR @metacpp, i've left some mostly minor comments inline. the major issue i see is the continuing instead of nested if blocks, i think it has the potential to miss elements and shouldn't be done. WDYT?

azurerm/resource_arm_network_profile.go Show resolved Hide resolved
azurerm/resource_arm_network_profile.go Outdated Show resolved Hide resolved
azurerm/resource_arm_network_profile.go Outdated Show resolved Hide resolved
azurerm/resource_arm_network_profile.go Show resolved Hide resolved
azurerm/resource_arm_network_profile.go Outdated Show resolved Hide resolved
azurerm/resource_arm_network_profile_test.go Outdated Show resolved Hide resolved
website/docs/r/network_profile.html.markdown Show resolved Hide resolved
website/docs/r/network_profile.html.markdown Outdated Show resolved Hide resolved
website/docs/r/network_profile.html.markdown Outdated Show resolved Hide resolved
website/docs/r/network_profile.html.markdown Outdated Show resolved Hide resolved
@metacpp
Copy link
Contributor Author

metacpp commented Apr 3, 2019

@katbyte Thanks for the time to reviewing the PR, I've addressed most of the comments in the PR.

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @metacpp

Thanks for pushing those changes; I've left some more comments inline; so that we can get this merged I'm going to push a commit to fix those, I hope you don't mind.

Thanks!

azurerm/resource_arm_network_profile.go Outdated Show resolved Hide resolved
website/docs/r/network_profile.html.markdown Show resolved Hide resolved
azurerm/resource_arm_network_profile.go Outdated Show resolved Hide resolved
azurerm/resource_arm_network_profile.go Outdated Show resolved Hide resolved
azurerm/resource_arm_network_profile.go Outdated Show resolved Hide resolved
azurerm/resource_arm_network_profile.go Outdated Show resolved Hide resolved
azurerm/resource_arm_network_profile.go Outdated Show resolved Hide resolved
@tombuildsstuff tombuildsstuff removed the request for review from JunyiYi May 13, 2019 14:54
@tombuildsstuff tombuildsstuff removed the request for review from WodansSon May 13, 2019 14:54
@tombuildsstuff tombuildsstuff added this to the v1.28.0 milestone May 14, 2019
@tombuildsstuff tombuildsstuff dismissed katbyte’s stale review May 14, 2019 10:02

dismissing since changes have been pushed

@tombuildsstuff
Copy link
Contributor

Tests pass:

Screenshot 2019-05-14 at 12 44 20

@tombuildsstuff tombuildsstuff merged commit b820fba into master May 14, 2019
@tombuildsstuff tombuildsstuff deleted the nw_profile branch May 14, 2019 11:34
tombuildsstuff added a commit that referenced this pull request May 14, 2019
@ghost
Copy link

ghost commented May 17, 2019

This has been released in version 1.28.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
	version = "~> 1.28.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Jun 13, 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 13, 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.

4 participants