Skip to content

Conversation

@amolr
Copy link

@amolr amolr commented Oct 25, 2016

Description

The create/update namespace operation for Notification Hubs can now use a SkuTier parameter which sets the Sku level at the namespace level. This change allows the user to set the Sku using the parameter which creating or updating the namespace. In case of create, the skuTier if not specified is free by default.


This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.

  • I have read the contribution guidelines.
  • **If changes were made to any cmdlet, the XML help was regenerated using the Powershell CmdLet Help Editor
  • If any large changes are made to a service, they are reflected in the respective change log.

General Guidelines

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.
  • The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

  • Pull request includes test coverage for the included changes.
  • PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources.

@azurecla
Copy link

Hi @amolr, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!


It looks like you're working at Microsoft (amolr). If you're full-time, we DON'T require a contribution license agreement.



If you are a vendor, DO please sign the electronic contribution license agreement. It will take 2 minutes and there's no faxing! https://cla.azure.com.

TTYL, AZPRBOT;

@azuresdkci
Copy link

Can one of the admins verify this patch?

1 similar comment
@azuresdkci
Copy link

Can one of the admins verify this patch?

@cormacpayne
Copy link
Member

@azuresdkci add to whitelist

@shahabhijeet
Copy link
Contributor

@amolr can you address failures

@cormacpayne
Copy link
Member

@amolr Hey Amol, can you add the following tests:

  • Using New-AzureRmNotificationHubsNamespace where the SkuTier is "Free" or isn't used at all
  • Using Set-AzureRmNotificationHubsNamespace where the SkuTier is a value other than "Free"

Copy link
Member

Choose a reason for hiding this comment

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

@amolr why was this change made?

Copy link
Author

Choose a reason for hiding this comment

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

I think this change got introduced while trying to debug an issue with Abhijeet related to our tests failing

Copy link
Member

Choose a reason for hiding this comment

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

@amolr please rename this to SkuTier

Copy link
Member

Choose a reason for hiding this comment

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

@amolr please rename this to SkuTier

Copy link
Member

Choose a reason for hiding this comment

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

@amolr does SkuTier need to have a default value here like it does in New-AzureNotificationHubsNamespace? For example, if someone passes in an empty string for SkuTier, does it need to be set to "free"?

Copy link
Author

Choose a reason for hiding this comment

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

No it need not have it here

@cormacpayne
Copy link
Member

@amolr please update the change log with the changes you have made in this PR

@markcowl
Copy link
Member

markcowl commented Nov 22, 2016

@amolr Please update the changelog as Cormac suggested, here: https://github.com/Azure/azure-powershell/blob/dev/src/ResourceManager/NotificationHubs/ChangeLog.md

Also, please clean up the commits, you have several commits titled 'regenerate wxi file'.. There are instructions for cleaning up commits here: https://github.com/Azure/azure-powershell/blob/dev/documentation/cleaning-up-commits.md

@markcowl
Copy link
Member

markcowl commented Nov 22, 2016

@amolr still looking for commit cleanup. Please remove the '[Cleup commits]' tag from the title when you have cleaned up the commits.

@markcowl markcowl changed the title Azure powershell SDK changes for Notification Hubs. Added a skuTier parameter for create/update namespace. [Cleanup commite]:Azure powershell SDK changes for Notification Hubs. Added a skuTier parameter for create/update namespace. Nov 22, 2016
@markcowl markcowl changed the title [Cleanup commite]:Azure powershell SDK changes for Notification Hubs. Added a skuTier parameter for create/update namespace. [Cleanup commits]:Azure powershell SDK changes for Notification Hubs. Added a skuTier parameter for create/update namespace. Nov 22, 2016
@amolr amolr changed the title [Cleanup commits]:Azure powershell SDK changes for Notification Hubs. Added a skuTier parameter for create/update namespace. Azure powershell SDK changes for Notification Hubs. Added a skuTier parameter for create/update namespace. Nov 23, 2016
@cormacpayne
Copy link
Member

LGTM :shipit:

@cormacpayne cormacpayne merged commit 626036a into Azure:dev Nov 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants