-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Support in-place update of app service slots #1436
Conversation
There was a problem hiding this 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 👍
One of the tests is failing:
|
7496e37
to
7062454
Compare
there are two of the older tests still failing with the same error:
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. |
Thanks for that @katbyte. It looks like the same tests are failing for the regular I'll try to find some time to debug the issue. |
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. |
@phekmat is updating App Service Slots dependent on the API upgrade? If not it may be worth removing that change for the moment? |
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
dd5ded5
to
298eabf
Compare
I've rebased and added the IP/mask translation, so this should be good for review again. Apologies for the delay |
There was a problem hiding this 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!
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:
Thanks! |
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! |
ForceNew
on several slot attributes and update them in placeFixes #1335