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

Added option to set the application security group of a VMSS #2009

Merged
merged 14 commits into from
Oct 26, 2018
Merged

Added option to set the application security group of a VMSS #2009

merged 14 commits into from
Oct 26, 2018

Conversation

chapmonkey
Copy link
Contributor

@chapmonkey chapmonkey commented Oct 2, 2018 •
edited by katbyte
Loading

No description provided.

@ghost ghost added the size/S label Oct 2, 2018
@ghost ghost added size/XS and removed size/S labels Oct 3, 2018
@ghost ghost added size/S and removed size/XS labels Oct 3, 2018
@ghost ghost added size/XS and removed size/S labels Oct 3, 2018
@ghost ghost added size/L and removed size/XS labels Oct 3, 2018
@chapmonkey
Copy link
Contributor Author

Is there anything else I need to do for this PR to be merged?

@ckrohn
Copy link

ckrohn commented Oct 10, 2018

When will this issue be merged? 😃

@chapmonkey chapmonkey mentioned this pull request Oct 10, 2018
Closed
tombuildsstuff
tombuildsstuff suggested changes Oct 12, 2018
View reviewed changes
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 @chapmonkey

Thanks for this PR :)

I've taken a look through and this mostly LGTM - if we can address the comments (and the tests pass) then we should be able to get this merged :)

Thanks!

azurerm/resource_arm_virtual_machine_scale_set_test.go Outdated
Config: config,
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMVirtualMachineScaleSetExists(resourceName),
testCheckAzureRMVirtualMachineScaleSetApplicationSecurity(resourceName),
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than reaching out and checking the state of the resource in Azure - we should be able to verify this in the local state by switching this to using a generic test check function like so:

resource.TestCheckResourceAttr(resourceName, "ip_configuration.0.application_security_group_ids.#", "1")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will update this test.

azurerm/resource_arm_virtual_machine_scale_set.go Show resolved Hide resolved
azurerm/resource_arm_virtual_machine_scale_set_test.go Outdated
ip_configuration {
name = "TestIPConfiguration"
subnet_id = "${azurerm_subnet.test.id}"
application_security_group_ids = ["${azurerm_application_security_group.test.id}"]
Copy link
Contributor

Choose a reason for hiding this comment

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

minor could we fix the indentation to match the rest of resource by using spaces instead of tabs here?

website/docs/r/virtual_machine_scale_set.html.markdown Outdated Show resolved Hide resolved
@tombuildsstuff tombuildsstuff added this to the Soon milestone Oct 12, 2018
katbyte
katbyte requested changes Oct 16, 2018
View reviewed changes
azurerm/resource_arm_virtual_machine_scale_set.go Show resolved Hide resolved
azurerm/resource_arm_virtual_machine_scale_set.go Outdated
"application_security_group_ids": {
Type: schema.TypeSet,
Optional: true,
Elem: &schema.Schema{Type: schema.TypeString},
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 also validate the ID with azure.ValidateResourceId?

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 will have a look

@chapmonkey
Copy link
Contributor Author

I have pushed my changes as request in the review

@chapmonkey
Copy link
Contributor Author

@tombuildsstuff Do you know what version this might end up in? We are having to use a workaround for this at the moment.

@tombuildsstuff tombuildsstuff modified the milestones: Soon, Being Sorted, 1.18.0 Oct 25, 2018
@katbyte katbyte changed the title Added option to set the application security group of a scale set ins… Added option to set the application security group of a VMSS Oct 26, 2018
@katbyte katbyte added service/vmss Virtual Machine Scale Sets and removed service/application-security-groups labels Oct 26, 2018
katbyte
katbyte approved these changes Oct 26, 2018
View reviewed changes
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.

Thanks for the updates @chapmonkey, aside from two minor comments this LGTM 👍

I hope you don't mind but i am going to push some changes so i can get this merged today 🙂 It should go out in 1.18 next week.

azurerm/resource_arm_virtual_machine_scale_set.go Outdated
applicationSecurityGroups := make([]interface{}, 0)
if properties.ApplicationSecurityGroups != nil {
for _, asg := range *properties.ApplicationSecurityGroups {
applicationSecurityGroups = append(applicationSecurityGroups, *asg.ID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

ID should be nil checked

website/docs/r/virtual_machine_scale_set.html.markdown Outdated Show resolved Hide resolved
@chapmonkey
Copy link
Contributor Author

@katbyte Ok, that would be great, thanks.

@katbyte
Copy link
Collaborator

katbyte commented Oct 26, 2018

test passes:

==> Fixing source code with gofmt...
gofmt -s -w ./azurerm
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -test.run=TestAccAzureRMVirtualMachineScaleSet_basicApplicationSecurity -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicApplicationSecurity
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basicApplicationSecurity (321.55s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	321.581s

@katbyte katbyte merged commit ebd4d6f into hashicorp:master Oct 26, 2018
@chapmonkey chapmonkey deleted the vmss-application-security-groups branch October 26, 2018 20:04
@tombuildsstuff
Copy link
Contributor

hi @chapmonkey @ckrohn

Just to let you know that this has been released as a part of v1.18 of the AzureRM Provider (the full changelog is available here). You can upgrade to this by specifying the version in the provider block (as shown below) and then running terraform init -upgrade

provider "azurerm" {
  version = "=1.18.0"
}

Thanks!

@ghost
Copy link

ghost commented Mar 6, 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 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 6, 2019
@ghost ghost removed the waiting-response label Mar 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tombuildsstuff tombuildsstuff tombuildsstuff requested changes

@katbyte katbyte katbyte approved these changes

Assignees

@katbyte katbyte

Labels
enhancement service/vmss Virtual Machine Scale Sets size/L
Projects
None yet
Milestone
v1.18.0
Development

Successfully merging this pull request may close these issues.
5 participants
@chapmonkey @ckrohn @katbyte @tombuildsstuff @metacpp