Skip to content

Resource Group - ManagedBy NULL/Empty String issue#4351

Closed
coolhome wants to merge 1 commit intoAzure:mainfrom
coolhome:ManagedResourceGRoup
Closed

Resource Group - ManagedBy NULL/Empty String issue#4351
coolhome wants to merge 1 commit intoAzure:mainfrom
coolhome:ManagedResourceGRoup

Conversation

@coolhome
Copy link
Copy Markdown
Contributor

@coolhome coolhome commented Dec 7, 2023

Description

Potential fix for #3386

This allowed me to omit the value. I have not tested lock module or the role assignment resource.

Unfortunately, this is a breaking change as managedBy is implicitly null and cannot be defaulted to an empty string. However, I think this may be desired since Azure CLI/PowerShell set this as null.

Please feel free to pick up and continue with testing.

Pipeline references

Pipeline

Type of Change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Update to documentation

Checklist

  • I'm sure there are no other open Pull Requests for the same update/change
  • My corresponding pipelines / checks run clean and green without any errors or warnings
  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (readme)
  • I did format my code

@coolhome coolhome requested a review from a team as a code owner December 7, 2023 19:35
@coolhome coolhome changed the title Potential workaround for #3386 Resource Group - ManagedBy NULL/Empty String issue Dec 7, 2023
@AlexanderSehr AlexanderSehr added the [cat] modules category: modules label Dec 7, 2023
@AlexanderSehr
Copy link
Copy Markdown
Contributor

Hey @coolhome, thanks for the PR :) Sorry that I didn't get around to it earlier - yet alone the linked issue. As the module is already migrated to AVM, @segraef, could you take a look at this? As @coolhome says, we should also test this out, but if it checks out, it would be great to get updated in the resource group module.

location: location
name: name
tags: tags
managedBy: length(managedBy ?? '') > 0 ? managedBy : ''
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
managedBy: length(managedBy ?? '') > 0 ? managedBy : ''
managedBy: managedBy ?? ''

If I'm not mistaken, this should do the trick to. Even better would be only mangedBy: mangedBy, IF null is accepted by the RP.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That is correct, but the RP does not accept null. If its json property exists it has to be a string, which makes this problem hard. This is why I've had challenges figuring out who to report this to CARML/AVM, Bicep, or Azure Support for RP.

I think the RP should accept null and require a string length greater than 0. Regardless it's a breaking change if someone used this module to create the resource group because it has to be deleted to remove managedBy AFAIK.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In the corresponding AVM module the owner now opted to not expose the ability to write to this property. This won't impact the RP but at least it wouldn't cause any other issues for users of the module.

@segraef
Copy link
Copy Markdown
Contributor

segraef commented Dec 10, 2023

Hey @coolhome, thanks for the PR :) Sorry that I didn't get around to it earlier - yet alone the linked issue. As the module is already migrated to AVM, @segraef, could you take a look at this? As @coolhome says, we should also test this out, but if it checks out, it would be great to get updated in the resource group module.

Testing ...

@segraef
Copy link
Copy Markdown
Contributor

segraef commented Dec 11, 2023

Alright, I tested some cases and all of them circle back to managedBy being an immutable string.
I reckon @AlexanderSehr introduced it in #2319 due to the API version update and just to have it included.

I simulated a managed resource group which gets its managedBy property by a service like Recovery Services Vault > Didn't work, once the rg is created the value is an immutable string.

As we all know a nullable string? doesn't work since managedBy would only accept an empty string but since it's an immutable string why would it anyway?

I've never seen (maybe someone else?) a use case where managedBy was used so hence I suggest to get rid of it instead of incorporating the suggested logic by @coolhome see Azure/bicep-registry-modules#718.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[cat] modules category: modules

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug Report]: resources/resource-group - ResourceGroupManagedByMismatch

3 participants