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

Support in-place update of app service slots #1436

Merged
merged 2 commits into from
Jul 15, 2018

Conversation

phekmat
Copy link
Contributor

@phekmat phekmat commented Jun 25, 2018

  • Upgrade to 2018-02-01 of the App Service API
  • Remove ForceNew on several slot attributes and update them in place

Fixes #1335

tombuildsstuff
tombuildsstuff previously approved these changes Jun 26, 2018
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 @phekmat

Thanks for this PR - this LGTM 👍

@tombuildsstuff tombuildsstuff added this to the 1.8.0 milestone Jun 26, 2018
@katbyte
Copy link
Collaborator

katbyte commented Jun 26, 2018

One of the tests is failing:

------- Stdout: -------
=== RUN   TestAccAzureRMAppServiceSlot_clientAffinityEnabledUpdate
--- FAIL: TestAccAzureRMAppServiceSlot_clientAffinityEnabledUpdate (157.39s)
    testing.go:513: Step 1 error: Check failed: Check 2/2 error: azurerm_app_service_slot.test: Attribute 'client_affinity_enabled' expected "false", got "true"
FAIL

@phekmat
Copy link
Contributor Author

phekmat commented Jun 27, 2018

@katbyte I pushed a fix for that (roughly the same as the one added in 61a1069)

@katbyte
Copy link
Collaborator

katbyte commented Jun 28, 2018

@phekmat,

there are two of the older tests still failing with the same error:

------- Stdout: -------
=== RUN   TestAccAzureRMAppServiceSlot_manyIpRestrictions
--- FAIL: TestAccAzureRMAppServiceSlot_manyIpRestrictions (108.74s)
    testing.go:513: Step 0 error: Error applying: 1 error(s) occurred:
        
        * azurerm_app_service_slot.test: 1 error(s) occurred:
        
        * azurerm_app_service_slot.test: web.AppsClient#CreateOrUpdateSlot: Failure sending request: StatusCode=0 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="BadRequest" Message="IpSecurityRestriction.IpAddress is invalid." Details=[{"Message":"IpSecurityRestriction.IpAddress is invalid."},{"Code":"BadRequest"},{"ErrorEntity":{"Code":"BadRequest","ExtendedCode":"51012","Message":"IpSecurityRestriction.IpAddress is invalid.","MessageTemplate":"{0} is invalid.","Parameters":["IpSecurityRestriction.IpAddress"]}}]
FAIL

------- Stdout: -------
=== RUN   TestAccAzureRMAppServiceSlot_oneIpRestriction
--- FAIL: TestAccAzureRMAppServiceSlot_oneIpRestriction (104.85s)
    testing.go:513: Step 0 error: Error applying: 1 error(s) occurred:
        
        * azurerm_app_service_slot.test: 1 error(s) occurred:
        
        * azurerm_app_service_slot.test: web.AppsClient#CreateOrUpdateSlot: Failure sending request: StatusCode=0 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="BadRequest" Message="IpSecurityRestriction.IpAddress is invalid." Details=[{"Message":"IpSecurityRestriction.IpAddress is invalid."},{"Code":"BadRequest"},{"ErrorEntity":{"Code":"BadRequest","ExtendedCode":"51012","Message":"IpSecurityRestriction.IpAddress is invalid.","MessageTemplate":"{0} is invalid.","Parameters":["IpSecurityRestriction.IpAddress"]}}]
FAIL

These seem to be new and unique to your PR. Apologies for missing these earlier. I just tested the ones you had added at first.

@phekmat
Copy link
Contributor Author

phekmat commented Jun 29, 2018

Thanks for that @katbyte. It looks like the same tests are failing for the regular app_service resource, so it's likely due to the new API version.

I'll try to find some time to debug the issue.

@phekmat
Copy link
Contributor Author

phekmat commented Jun 29, 2018

Minor update, it looks like the new version of the API has both removed the subnetMask field (or rather, throws back a 400 if it's included) and requires that the IP address be in CIDR notation (x.x.x.x/y). I couldn't find any documentation of this new behavior, but it should be straightforward to work around it.

@tombuildsstuff
Copy link
Contributor

@phekmat is updating App Service Slots dependent on the API upgrade? If not it may be worth removing that change for the moment?

@phekmat
Copy link
Contributor Author

phekmat commented Jul 2, 2018

The API upgrade is required IIRC. The Swagger was updated for the old API for web apps in Azure/azure-rest-api-specs#2664, but those fields are still there for slots. I believe I ran into the same issue as you did in Azure/azure-rest-api-specs#1697

- Upgrade to 2018-02-01 of the App Service API
- Remove `ForceNew` on several slot attributes and update them in place
- Translate between CIDR-format IP addresses (a.b.c.d/x) and IP/subnet
mask combination
- Add additional test IP restriction to capture CIDR address translation
@phekmat
Copy link
Contributor Author

phekmat commented Jul 12, 2018

I've rebased and added the IP/mask translation, so this should be good for review again. Apologies for the delay

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 @phekmat

Thanks for pushing the latest changes here - I've taken a look through and this now LGTM, I'll kick off the tests suite now 👍🏻

Thanks!

@tombuildsstuff
Copy link
Contributor

All the App Service related tests pass:

screenshot 2018-07-15 at 13 38 50

@tombuildsstuff tombuildsstuff merged commit d3f9cdd into hashicorp:master Jul 15, 2018
tombuildsstuff added a commit that referenced this pull request Jul 15, 2018
@tombuildsstuff
Copy link
Contributor

tombuildsstuff commented Jul 21, 2018

hi @phekmat

Just to let you know that is now available in v1.10 of the AzureRM Provider - which you can update to this version by specifying it in the Provider block, like so:

provider "azurerm" {
  version = "=1.10.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
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