-
Notifications
You must be signed in to change notification settings - Fork 4k
Azure RM Dns Bug Fixes and Update to Help File #2835
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
Conversation
Signed-off-by: Corina <[email protected]>
Signed-off-by: Corina <[email protected]>
Signed-off-by: Corina <[email protected]>
Signed-off-by: Corina <[email protected]>
…Sets Signed-off-by: Corina <[email protected]>
Signed-off-by: Corina <[email protected]>
Signed-off-by: Corina <[email protected]>
…e is SOA Signed-off-by: Corina <[email protected]>
Signed-off-by: Corina <[email protected]>
Signed-off-by: Corina <[email protected]>
Signed-off-by: Corina <[email protected]>
…nd Confirm Impact where applicable; modifies tests to not use -Force Signed-off-by: Corina <[email protected]>
Hi @corinajacobson, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!
TTYL, AZPRBOT; |
Can one of the admins verify this patch? |
@corinajacobson, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
@@ -215,6 +215,7 @@ | |||
<None Include="..\..\Common\Commands.ScenarioTests.ResourceManager.Common\AzureRM.Resources.ps1"> | |||
<Link>ScenarioTests\AzureRM.Resources.ps1</Link> | |||
</None> | |||
<None Include="app.config" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove, this will cause failures, ass it is on .gitignore
@azuresdkci add to whitelist |
All comments have been addressed and I will push to pull request after running tests one more time. Thank you! |
@@ -61,15 +61,17 @@ public class NewAzureDnsRecordSet : DnsBaseCmdlet | |||
[Parameter(Mandatory = false, HelpMessage = "Do not fail if the record set already exists.")] | |||
public SwitchParameter Overwrite { get; set; } | |||
|
|||
[Parameter(Mandatory = false, HelpMessage = "Do not ask for confirmation.")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change. The advice for this breaking change is to leave the parameeter add the [Obsolete(...)]
attribute to the property, and not use it in cmdlet execution. This gives users time to adapt to the change.
@corinajacobson Sorry, I had forgotten about the breaking change policy, so missed this last time - weh should leave the Force parameters, not use them, and makr them as obsolete - this gives users a chance to update their scripts. |
@markcowl Does the breaking change policy apply to services in Preview? |
@jtuliani Right now there is no preview for powershell. WHen there is, it will not, but for now, there is only one way for customers to get azure powershell |
on demand run here:http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-demand/1135/ LGTM once the build passes |
@markcowl The Azure DNS service is in Preview. As with other Azure Previews, we clearly state that it is provided without SLA, and without Azure Support, should not be used for production workloads, and is subject to a 50% billing discount. To me, 'Preview' includes every part of the service, including the PowerShell cmdlets and CLI. I therefore don't think the breaking change policy should apply (indeed, we have made other breaking changes during Preview, although we prefer not to of course). |
Looks like tests have passed! Please let me know if I can do anything else to help out. |
@jtuliani Unfortunately, we don't have any current way of differentiating the preview from the stable modules in azure powershell. Expect some discussion of new policy in this area over the next couple of weeks. |
This checklist is used to make sure that common issues in a pull request are covered by the creator. You can find a more complete discussion of PowerShell cmdlet best practices here.
Below in Overall Changes, check off the boxes that apply to your PR. Within each of the categories that you select, make sure that you can check off all of the boxes.
Overall Changes
General
Tests
Cmdlet Signature
VerbsCommon
,VerbsCommunication
,VerbsLifecycle
,VerbsOther
whenever possibleOutputType
attribute if any output is produced; if the cmdlet produces no output, it should implement aPassThrough
parameterShouldProcess
and haveSupportShouldProcess = true
specified in the cmdlet attribute. See a discussion about correctShouldProcess
implementation hereDefaultParameterSetName
in its cmdlet attributeParameters
object
.ResourceGroup
type string marked as [ValueFromPipelineByPropertyName]ResourceName
type string marked as [ValueFromPipelineByPropertyName]HashTable
Parameters and the Pipeline
object
ResourceGroupName
andName
from the pipeline by property valueGet
andNew
cmdlets to the input ofSet
,Update
,Remove
and other action cmdlets for that resource