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

r/role_definition: fixing a crash when permissions is nil & making permissions optional #9850

Merged
merged 11 commits into from
Mar 25, 2021

Conversation

tombuildsstuff
Copy link
Contributor

@tombuildsstuff tombuildsstuff commented Dec 14, 2020

In testing it appears this block can be left empty (e.g. []) - since the API will transform an empty permissions block into []

Fixes #9815
Fixes #440
Fixes #3732

Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@tombuildsstuff tombuildsstuff added this to the v2.49.0 milestone Feb 17, 2021
@tombuildsstuff tombuildsstuff modified the milestones: v2.49.0, v2.51.0 Feb 23, 2021
@tombuildsstuff tombuildsstuff modified the milestones: v2.51.0, v2.52.0 Mar 11, 2021
@jackofallops jackofallops modified the milestones: v2.52.0, v2.53.0 Mar 18, 2021
@jackofallops jackofallops marked this pull request as ready for review March 25, 2021 08:34
tombuildsstuff and others added 10 commits March 25, 2021 17:17
The API returns an empty list if an empty permissions block is sent
but also allows sending an empty list - so it appears that this isn't
required.
…ttled down

Turns out the Update call is eventually consistent - where a new Role Definition
gets created and then reconciled on the backend. This new Role Definition has a
new CreatedOn and UpdatedOn, so it's possible to determine when this has been
updated - however in testing it can take ~5m for this to settle down.

Whilst it's unfortunate this takes a little longer to settle down, this should
improve the reliability of resources which depend on this.

Unfortunately the Azure SDK doesn't expose these fields in the response, so for
now we're using a wrapped to access them and confirm this has been updated.
@ghost ghost added the dependencies label Mar 25, 2021
@jackofallops
Copy link
Member

Tests look good:

image

@jackofallops jackofallops merged commit 13d2b3c into master Mar 25, 2021
jackofallops added a commit that referenced this pull request Mar 25, 2021
@ghost
Copy link

ghost commented Mar 26, 2021

This has been released in version 2.53.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 = "~> 2.53.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Apr 25, 2021

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 as resolved and limited conversation to collaborators Apr 25, 2021
@katbyte katbyte deleted the b/role-definition-crash-9815 branch May 25, 2021 18:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.