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

Add Managed Identity support to CdnProfile module #1621

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

nirarg
Copy link
Collaborator

@nirarg nirarg commented Jul 3, 2024

Changed module: azure_rm_cdnprofile

SUMMARY

Add Managed Identity support to CdnProfile module

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

azure_rm_cdnprofile

@nirarg nirarg added the work in In trying to solve, or in working with contributors label Jul 3, 2024
@nirarg
Copy link
Collaborator Author

nirarg commented Jul 3, 2024

There is still an issue with using append=false in the user identity
The module works as expected but its seems that the API still performing append instead of overwrite
I'm investigating this issue

@Fred-sun Fred-sun added the question Further information is requested label Jul 9, 2024
@nirarg
Copy link
Collaborator Author

nirarg commented Jul 9, 2024

Hi @Fred-sun,

Can you please advise on an issue I'm encountering in this PR?

When I update the Identity with "append=false", I expect the user identity to be replaced with the new one, not appended. However, the new task I added to the integration test in this PR (task name: "Update the CDN profile - replace user identity") doesn't return the expected result.

While debugging this, I couldn't find anything suspicious that could be causing this issue. I would appreciate any help you can provide.

Thank you!

curr_identity = response["identity"] if response else None
update_identity = False
if self.identity:
update_identity, self.identity = self.update_managed_identity(curr_identity=curr_identity, new_identity=self.identity)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@nirarg How did you set 'append=False'? The 'allow_identities_append=True' parameter of the function self.update_managed_identity has a default value, so it is added. If you want to override, you need to pass the function when 'allow_identities_append=False'. So you should pass it to the function according to the arguments of 'append'. Thank you!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @Fred-sun,
Yes, I used append=false. The allow_identities_append variable is used for cases where the append attribute is not available. This is not the case here, append is being used. Therefore, the default value in the function is allow_identities_append=True.
I debugged the code and saw that the identity is returned as expected from this function and the result is sent to the API. Therefore, I suspect there is something I miss in the API call or in the SDK code.

@p3ck
Copy link
Collaborator

p3ck commented Jul 16, 2024

I have this passing with the included integration tests when using the fixes I pushed in #1635 . You just need to update this pr with the following:

diff --git a/plugins/modules/azure_rm_cdnprofile.py b/plugins/modules/azure_rm_cdnprofile.py
index 0b371d2..eeec910 100644
--- a/plugins/modules/azure_rm_cdnprofile.py
+++ b/plugins/modules/azure_rm_cdnprofile.py
@@ -197,7 +197,7 @@ class AzureRMCdnprofile(AzureRMModuleBaseExt):
             curr_identity = response["identity"] if response else None
             update_identity = False
             if self.identity:
-                update_identity, self.identity = self.update_managed_identity(curr_identity=curr_identity, new_identity=self.identity)
+                update_identity, self.identity = self.update_managed_identity(curr_identity=curr_identity, new_identity=self.identity, patch_support=True)
 
             if not response:
                 self.log("Need to create the CDN profile")

The reason seems to be that some of the API calls (update) use the PATCH method. The backend logic will append to the list unless you pass keys with a value of NONE, then they will be removed.

Changed module: azure_rm_cdnprofile
@nirarg
Copy link
Collaborator Author

nirarg commented Jul 23, 2024

The verification CI test was started here:
https://dev.azure.com/azclitools/public/_build/results?buildId=174991&view=results

@Fred-sun Fred-sun added ready_for_review The PR has been modified and can be reviewed and merged medium_priority Medium priority new_feature New feature requirments and removed question Further information is requested work in In trying to solve, or in working with contributors labels Jul 24, 2024
@nirarg
Copy link
Collaborator Author

nirarg commented Aug 28, 2024

Hi @Fred-sun @xuzhang3
Is there any update regarding this PR review?
Is there anything else required to complete here before merge?

@xuzhang3 xuzhang3 merged commit 57d7806 into ansible-collections:dev Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
medium_priority Medium priority new_feature New feature requirments ready_for_review The PR has been modified and can be reviewed and merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants