dependenciess: - go-azure-sdk to version v0.20250617.1143239
#1722
Conversation
|
Test results: Administrative Units: App Role Assignments: Conditional Access (new failures, will need to investigate): Directory Objects: Directory Roles: Domains Groups (new failed tests are flaky, low success rate on main): Identity Governance: Invitations: Policies: Service Principals: Synchronization: User Flows: Users: |
There was a problem hiding this comment.
Hi @jackofallops, there are numerous test failures that seem to be related to the deletion of a workaround in Pandora (in this commit)
I've pointed out a few in this PR, but my assumption is all that were present in that workaround will need to be nullable for the resources to function as expected.
| "membership_kind": externalTenants.MembershipKind, | ||
| "members": tf.FlattenStringSlicePtr(externalTenants.Members), | ||
| }, | ||
| if ext, ok := in.(stable.ConditionalAccessEnumeratedExternalTenants); ok { |
There was a problem hiding this comment.
The addition of this type assertion is causing a diff if external_tenants.membership_kind = "all", this flatten func will need to be updated to set membership_kind when type is ConditionalAccessAllExternalTenants
| result.ApplicationEnforcedRestrictions = &stable.ApplicationEnforcedRestrictionsSessionControl{ | ||
| IsEnabled: nullable.Value(config["application_enforced_restrictions_enabled"].(bool)), | ||
| } | ||
|
|
There was a problem hiding this comment.
The changes from PR #1719 are being reverted in this PR, causing test failures
| result.ApplicationEnforcedRestrictions = &stable.ApplicationEnforcedRestrictionsSessionControl{ | |
| IsEnabled: nullable.Value(config["application_enforced_restrictions_enabled"].(bool)), | |
| } |
| DisableResilienceDefaults := config["disable_resilience_defaults"] | ||
| result.DisableResilienceDefaults = nullable.Value(DisableResilienceDefaults.(bool)) | ||
|
|
There was a problem hiding this comment.
Same as above
| DisableResilienceDefaults := config["disable_resilience_defaults"] | |
| result.DisableResilienceDefaults = nullable.Value(DisableResilienceDefaults.(bool)) |
| signInFrequency.Type = pointer.To(stable.SigninFrequencyType(config["sign_in_frequency_period"].(string))) | ||
| signInFrequency.Value = nullable.Value(int64(frequencyValue)) | ||
|
|
||
| // AuthenticationType and FrequencyInterval must be set to default values here |
There was a problem hiding this comment.
This was removed in #1719, not sure it should be added back?
| // AuthenticationType and FrequencyInterval must be set to default values here |
| signInFrequency.AuthenticationType = pointer.To(stable.SignInFrequencyAuthenticationType(authenticationType.(string))) | ||
| } | ||
|
|
||
| if interval, ok := config["sign_in_frequency_interval"]; ok && interval.(string) != "" { |
There was a problem hiding this comment.
Reverted #1719, I assume that was unintentional?
| if interval, ok := config["sign_in_frequency_interval"]; ok && interval.(string) != "" { | |
| if interval, ok := config["sign_in_frequency_interval"]; ok && interval.(string) != "" { | |
| signInFrequency.IsEnabled = nullable.Value(true) | |
| signInFrequency.AuthenticationType = pointer.To(stable.SignInFrequencyAuthenticationType_PrimaryAndSecondaryAuthentication) | |
| if authType := config["sign_in_frequency_authentication_type"].(string); authType != "" { | |
| signInFrequency.AuthenticationType = pointer.ToEnum[stable.SignInFrequencyAuthenticationType](authType) | |
| } |
| applicationEnforcedRestrictions := config["application_enforced_restrictions_enabled"].(bool) | ||
| if pointer.From(signInFrequency.FrequencyInterval) != stable.SignInFrequencyInterval_EveryTime { // application enforced restrictions are not allowed for everyTime sign-in frequency see https://github.com/hashicorp/terraform-provider-azuread/issues/1225 | ||
| result.ApplicationEnforcedRestrictions = &stable.ApplicationEnforcedRestrictionsSessionControl{ | ||
| IsEnabled: nullable.Value(applicationEnforcedRestrictions), | ||
| } | ||
| } | ||
|
|
||
| DisableResilienceDefaults := config["disable_resilience_defaults"].(bool) | ||
| if pointer.From(signInFrequency.FrequencyInterval) != stable.SignInFrequencyInterval_EveryTime { // disable resilience defaults are not allowed for everyTime sign-in frequency see https://github.com/hashicorp/terraform-provider-azuread/issues/1225 | ||
| result.DisableResilienceDefaults = nullable.Value(DisableResilienceDefaults) | ||
| } | ||
|
|
There was a problem hiding this comment.
Unintentionally removed?
| @@ -20,7 +20,7 @@ type ConditionalAccessUsers struct { | |||
| IncludeGroups *[]string `json:"includeGroups,omitempty"` | |||
|
|
|||
| // Internal guests or external users included in the policy scope. Optionally populated. | |||
| IncludeGuestsOrExternalUsers *ConditionalAccessGuestsOrExternalUsers `json:"includeGuestsOrExternalUsers"` | |||
| IncludeGuestsOrExternalUsers *ConditionalAccessGuestsOrExternalUsers `json:"includeGuestsOrExternalUsers,omitempty"` | |||
There was a problem hiding this comment.
This change is problematic and causing test failures
TestAccConditionalAccessPolicy_guestsOrExternalUsers with error:
unexpected status 400 (400 Bad Request) with error: BadRequest: 1119: Users
assignment value 'None', 'All' or 'GuestsOrExternalUsers' cannot be combined
with guestOrExternalUsers. Please remove one of the assignment and try again.
For examples, please see API documentation at
https://docs.microsoft.com/en-us/graph/api/conditionalaccesspolicy-update?view=graph-rest-1.0.
At first glance my assumption is that it will need to be nullable
| @@ -8,7 +8,7 @@ type ConditionalAccessUsers struct { | |||
| ExcludeGroups *[]string `json:"excludeGroups,omitempty"` | |||
|
|
|||
| // Internal guests or external users excluded from the policy scope. Optionally populated. | |||
| ExcludeGuestsOrExternalUsers *ConditionalAccessGuestsOrExternalUsers `json:"excludeGuestsOrExternalUsers"` | |||
| ExcludeGuestsOrExternalUsers *ConditionalAccessGuestsOrExternalUsers `json:"excludeGuestsOrExternalUsers,omitempty"` | |||
There was a problem hiding this comment.
While I didn't see a test failure related to this property, my assumption is that the behaviour is likely the same as IncludeGuestsOrExternalUsers and that this will need to be nullable
|
|
||
| // Insider risk levels included in the policy. The possible values are: minor, moderate, elevated, unknownFutureValue. | ||
| InsiderRiskLevels *ConditionalAccessInsiderRiskLevels `json:"insiderRiskLevels,omitempty"` | ||
|
|
||
| // Locations included in and excluded from the policy. | ||
| Locations *ConditionalAccessLocations `json:"locations"` | ||
| Locations *ConditionalAccessLocations `json:"locations,omitempty"` |
There was a problem hiding this comment.
This needs to be nullable, causing a test failure for TestAccConditionalAccessPolicy_includedUserActions
|
Closing as updated/fixed version on the way. |
2 participants
No description provided.