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

feat: Add Remove-AzApiManagementUser script #321

Merged
merged 4 commits into from
Aug 18, 2022

Conversation

pim-simons
Copy link
Contributor

Added script to remove a user to an Azure API Management service.
Closes #320

Note: cannot add integration tests as we don't have an Azure API Management service available in our resource group due to costs.

@pim-simons pim-simons added feature All issues related to new features area:api-management All issues related to Azure API Management labels Aug 17, 2022
@netlify
Copy link

netlify bot commented Aug 17, 2022

Deploy Preview for arcus-scripting ready!

Name Link
🔨 Latest commit ffbcf51
🔍 Latest deploy log https://app.netlify.com/sites/arcus-scripting/deploys/62fdd9c84409f20009a2b433
😎 Deploy Preview https://deploy-preview-321--arcus-scripting.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@pim-simons
Copy link
Contributor Author

The name of the new script is Remove-AzApiManagementUser, as this follows our earlier scripts like Create-AzApiManagementUser for the user part and Remove-AzApiManagementDefaults for the remove part.

However this is also the name of the script in the Az.ApiManagement module, see https://docs.microsoft.com/en-us/powershell/module/az.apimanagement/remove-azapimanagementuser?view=azps-8.2.0.

This caused some issues during unit testing, so some commands have the module specified, such as:
https://github.com/arcus-azure/arcus.scripting/pull/321/files#diff-e1b36c2bbfb0483eb2ed4e8fd8e53e5497845e4459253c4cc182c14361734f11R1389
https://github.com/arcus-azure/arcus.scripting/pull/321/files#diff-e1b36c2bbfb0483eb2ed4e8fd8e53e5497845e4459253c4cc182c14361734f11R1406
https://github.com/arcus-azure/arcus.scripting/pull/321/files#diff-59aa3eb8ad259606b36e109cb8c6a4f4b532a886be3bff485dcbd01806da1739R31

During all my testing this wasn't an issue, but interested in if you think we should rename or not @stijnmoreels

@stijnmoreels
Copy link
Member

stijnmoreels commented Aug 18, 2022

The name of the new script is Remove-AzApiManagementUser, as this follows our earlier scripts like Create-AzApiManagementUser for the user part and Remove-AzApiManagementDefaults for the remove part.

However this is also the name of the script in the Az.ApiManagement module, see https://docs.microsoft.com/en-us/powershell/module/az.apimanagement/remove-azapimanagementuser?view=azps-8.2.0.

This caused some issues during unit testing, so some commands have the module specified, such as: https://github.com/arcus-azure/arcus.scripting/pull/321/files#diff-e1b36c2bbfb0483eb2ed4e8fd8e53e5497845e4459253c4cc182c14361734f11R1389 https://github.com/arcus-azure/arcus.scripting/pull/321/files#diff-e1b36c2bbfb0483eb2ed4e8fd8e53e5497845e4459253c4cc182c14361734f11R1406 https://github.com/arcus-azure/arcus.scripting/pull/321/files#diff-59aa3eb8ad259606b36e109cb8c6a4f4b532a886be3bff485dcbd01806da1739R31

During all my testing this wasn't an issue, but interested in if you think we should rename or not @stijnmoreels

Yeah, I think that we should rename this instead then. PowerShell let's you use it together with -AlloClobber but don't think this is a good approach, and I imagine many 'strange' behaviors if the users use the Azure variant. We also used Create-AzTableStorageAccountTable to avoid calling it New-AzStorageTable in the Table Storage module.

Maybe we can add ...Account to it? Like Create-AzApiManagementUserAccount and Remove-AzApiManagementUserAccount? (it's maybe even more clear then)
It's fine if the first one (create) is done in another PR to keep this one clean.

@pim-simons
Copy link
Contributor Author

The name of the new script is Remove-AzApiManagementUser, as this follows our earlier scripts like Create-AzApiManagementUser for the user part and Remove-AzApiManagementDefaults for the remove part.
However this is also the name of the script in the Az.ApiManagement module, see docs.microsoft.com/en-us/powershell/module/az.apimanagement/remove-azapimanagementuser?view=azps-8.2.0.
This caused some issues during unit testing, so some commands have the module specified, such as: #321 (files) #321 (files) #321 (files)
During all my testing this wasn't an issue, but interested in if you think we should rename or not @stijnmoreels

Yeah, I think that we should rename this instead then. PowerShell let's you use it together with -AlloClobber but don't think this is a good approach, and I imagine many 'strange' behaviors if the users use the Azure variant. We also used Create-AzTableStorageAccountTable to avoid calling it New-AzStorageTable in the Table Storage module.

Maybe we can add ...Account to it? Like Create-AzApiManagementUserAccount and Remove-AzApiManagementUserAccount? (it's maybe even more clear then) It's fine if the first one (create) is done in another PR to keep this one clean.

Renamed the scripts to Create-AzApiManagementUserAccount and Remove-AzApiManagementUserAccount 👍🏻 Included the renaming of the create script in this PR as well, as I feel this is related to this PR (renaming only came up during development of the remove script). Thanks for your feedback!

Copy link
Member

@stijnmoreels stijnmoreels left a comment

Choose a reason for hiding this comment

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

Great! v0.7 is shaping to be an amazing release. Thx for this!

@stijnmoreels stijnmoreels merged commit 2c31e34 into arcus-azure:master Aug 18, 2022
@pim-simons pim-simons deleted the feature/apim-remove-users branch August 18, 2022 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api-management All issues related to Azure API Management feature All issues related to new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide script to remove users from Azure APIM
2 participants