Skip to content

ApiManagement Resource Provider Zone Changes #15472

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

ApiManagement Resource Provider Zone Changes #15472

wants to merge 1 commit into from

Conversation

solankisamir
Copy link
Member

@solankisamir solankisamir commented Jul 14, 2021 •
edited
Loading

Description

Moved to 7.0.0 version of .NET SDK https://www.nuget.org/packages/Microsoft.Azure.Management.ApiManagement/7.0.0-preview

  • Updated cmdlet New-AzApiManagement to manage ApiManagement service

    • Added support for the new Isolated SKU
    • Added support for managing Availability Zones using Zone property
    • Added support for Disabling Gateway in a Region using DisableGateway property
    • Added support for managing the minimum Api Version to allow for Control Plane using ApiVersionConstraint property.

  • Updated cmdlet New-AzApiManagementRegion to manage ApiManagement service

    • Added support for managing Availability Zones using Zone property
    • Added support for Disabling Gateway in a Region using DisableGateway property

  • Updated cmdlet Add-AzApiManagementRegion to manage ApiManagement service

    • Added support for managing Availability Zones using Zone property
    • Added support for Disabling Gateway in a Region using DisableGateway property

  • Updated cmdlet Update-AzApiManagementRegion to manage ApiManagement service

    • Added support for managing Availability Zones using Zone property
    • Added support for Disabling Gateway in a Region using DisableGateway property

  • Updated cmdlet New-AzApiManagementCustomHostnameConfiguration to manage Custom Hostname Configuration

    • Added support for specifying IdentityClientId to provide Managed Identity User Assigned ClientId to use with KeyVault

Checklist

  • I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:

@solankisamir solankisamir changed the title ApiManagement Zone Changes [Draft] ApiManagement Zone Changes Jul 14, 2021
@isra-fel
Copy link
Member

Let me move the pull request to draft state. Please feel free to mark it "Ready for review" when you feel comfortable. Thanks

@isra-fel isra-fel marked this pull request as draft July 15, 2021 01:36
@solankisamir solankisamir changed the title [Draft] ApiManagement Zone Changes ApiManagement Resource Provider Zone Changes Jul 16, 2021
@solankisamir solankisamir requested a review from KacieKK July 16, 2021 22:05
@solankisamir solankisamir force-pushed the sasolank/2020-12-01-cmdlets branch from 30c56fb to 7145522 Compare July 16, 2021 22:18
KacieKK
KacieKK approved these changes Jul 16, 2021
View reviewed changes
Copy link
Contributor

@KacieKK KacieKK left a comment

Choose a reason for hiding this comment

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

:shipit:

@isra-fel isra-fel self-assigned this Jul 18, 2021
isra-fel
isra-fel reviewed Jul 18, 2021
View reviewed changes
src/ApiManagement/ApiManagement.Test/ApiManagement.Test.csproj
@@ -11,7 +11,7 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Microsoft.Azure.Management.ApiManagement" Version="6.0.0-preview" />
<PackageReference Include="Microsoft.Azure.Management.ApiManagement" Version="7.0.0-preview" />
Copy link
Member

Choose a reason for hiding this comment

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

There are also references to ApiManagement SDK in ApiManagement.ServiceManagement.csproj and its test project. Please update them all.

Copy link
Member Author

Choose a reason for hiding this comment

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

@KacieKK is working on that project. She will publish a PR soon!

Is this okay?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry but I'm afraid then both of your PRs will be blocked.
All dependencies to the same libaray should be updated in one PR, otherwise there will be assembly loading conflicts at runtime, never to mention what if only one of your PRs makes into the release.

Can you update just the reference for her in your PR? Or will it make it unable to compile?

@isra-fel isra-fel marked this pull request as ready for review July 18, 2021 12:32
isra-fel
isra-fel requested changes Jul 20, 2021
View reviewed changes
src/ApiManagement/ApiManagement/Commands/NewAzureApiManagementCustomHostnameConfiguration.cs
ValueFromPipelineByPropertyName = false,
ParameterSetName = SslCertificateFromKeyVault,
Mandatory = false,
HelpMessage = "User Assigned Managed Identity ClientId credentials to authenticate to KeyVault to fetch Custom SSL Certificate.")]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
HelpMessage = "User Assigned Managed Identity ClientId credentials to authenticate to KeyVault to fetch Custom SSL Certificate.")]
HelpMessage = "User-assigned managed identity client ID to authenticate to KeyVault to fetch Custom SSL Certificate.")]

Client ID is not credential so I suggest remove that word

src/ApiManagement/ApiManagement/Commands/AddAzureApiManagementRegion.cs
Comment on lines +65 to +70
[Parameter(
Mandatory = false,
HelpMessage = "Flag only meant to be used for Premium SKU ApiManagement Service and Non Internal VNET deployments. " +
"This is useful in case we want to take a gateway region out of rotation." +
" This can also be used to standup a new region in Passive mode, test it and then make it Live later.")]
public bool? DisableGateway { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

We should also mention the default behavior when DIsableGateway is omitted.

src/ApiManagement/ApiManagement/Commands/NewAzureApiManagement.cs
[Parameter(
Mandatory = false,
HelpMessage = "Control Plane Apis version constraint for the API Management service.")]
public string ApiVersionConstraint { get; set; }
Copy link
Member

@isra-fel isra-fel Jul 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We should describe the format of ApiVersionConstraint. It's not obvious to me whether it's a single version or a range or even an expression

@isra-fel isra-fel assigned solankisamir and unassigned isra-fel Jul 21, 2021
@isra-fel
Copy link
Member

I see this PR is combined into #15524 so let me close it

@isra-fel isra-fel closed this Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isra-fel isra-fel isra-fel requested changes

@KacieKK KacieKK KacieKK approved these changes

Assignees

@solankisamir solankisamir

Labels
needs-revision
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@solankisamir @isra-fel @KacieKK