-
Notifications
You must be signed in to change notification settings - Fork 4.1k
[Synapse] Add integration runtime management cmdlets #12647
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
|
Can one of the admins verify this patch? |
| "Microsoft.Azure.PowerShell.Cmdlets.Synapse.dll","Microsoft.Azure.Commands.Synapse.SubmitAzureSynapseSparkJob","Submit-AzSynapseSparkJob","1","8100","Submit-AzSynapseSparkJob Does not support ShouldProcess but the cmdlet verb Submit indicates that it should.","Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue" | ||
| "Microsoft.Azure.PowerShell.Cmdlets.Synapse.dll","Microsoft.Azure.Commands.Synapse.StartAzureSynapseSparkSession","Start-AzSynapseSparkSession","1","8100","Start-AzSynapseSparkSession Does not support ShouldProcess but the cmdlet verb Start indicates that it should.","Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue" | ||
| "Microsoft.Azure.PowerShell.Cmdlets.Synapse.dll","Microsoft.Azure.Commands.Synapse.InvokeAzureSynapseSparkStatement","Invoke-AzSynapseSparkStatement","1","8100","Invoke-AzSynapseSparkStatement Does not support ShouldProcess but the cmdlet verb Invoke indicates that it should.","Determine if the cmdlet should implement ShouldProcess and if so determine if it should implement Force / ShouldContinue" |
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.
Please take the suggested action to support shouldprocess for these 3 cmdlets
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.
@isra-fel These three top issues are existing issues. Can we just suppress these static analysis results because I think the current implementation (without ShouldProcess) are better in user experience?
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.
Supporting shouldprocess means adding two optional parameters -WhatIf and -Confirm. They do not affect default behavior of the cmdlet, but only provide additional information or confirmation when user is uncertain about it. So it is definitely better to support.
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.
I see. Thanks for your suggestions. I will fix this in a separate PR.
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.
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.
isra-fel
left a comment
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.
Please write test cases.
Have written the test cases. :) |
isra-fel
left a comment
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.
LGTM
Description
This change add integration runtime management cmdlets. Here is the commandlet review PR:https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/697
Checklist
CONTRIBUTING.mdChangeLog.mdfile(s) has been updated:ChangeLog.mdfile can be found atsrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md## Upcoming Releaseheader -- no new version header should be added