-
Notifications
You must be signed in to change notification settings - Fork 4k
ADL Updates to latest SDK in preparation for GA #3152
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
This enables us to test out a fix for download throttling scenarios. update the package to have more robust logic This gives more retries and better error messages.
Encryption Trusted ID providers Credential management Firewall rule management. Bug fixes for documentation. Updates for upload and download performance. Credential updates for internal testing. missed one file. add should process support for static analysis This makes static analysis happy and, for these cases, should be fine to include. update ga candidate private package and enable list credentials.
All packages are published now and should be pulled from nuget.
This will give users a heads up that all of this is changing/going away.
@markcowl is there anything else needed here to make sure this gets merged into the release? |
|
||
namespace Microsoft.Azure.Commands.DataLakeAnalytics | ||
{ | ||
[Cmdlet(VerbsCommon.New, "AzureRmDataLakeAnalyticsCatalogCredential", SupportsShouldProcess = true), OutputType(typeof(USqlCredential))] |
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.
Since there are multiple parameter sets, need a default parameter set name
@@ -22,6 +22,7 @@ namespace Microsoft.Azure.Commands.DataLakeAnalytics | |||
{ | |||
[Cmdlet(VerbsCommon.New, "AzureRmDataLakeAnalyticsCatalogSecret"), OutputType(typeof(USqlSecret))] | |||
[Alias("New-AdlCatalogSecret")] | |||
[Obsolete("Catalog secrets are being deprecated in the next release. Please use New-AzureRmDataLakeAnalyticsCatalogCredential directly instead.")] |
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.
...in a future release...
[ValidateNotNull] | ||
public PSCredential Password { get; set; } | ||
|
||
[Parameter(Position = 4, 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.
Switch parameters should not be positional.
Also, do you need a Force parameter? If there aren't special conditions you want to prompt the user about, consider not having a Force parameter.
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 maintain the force parameter to keep consistent with how the secret functionality prompts, for now, since it is the replacement. There shouldn't be an issue with removing it though, since credential removal doesn't have cascading ramifications. Thoughts there?
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 just chose to remove it, no force required for remove credential.
using System.Management.Automation; | ||
|
||
namespace Microsoft.Azure.Commands.DataLakeAnalytics | ||
{ | ||
[Cmdlet(VerbsCommon.Remove, "AzureRmDataLakeAnalyticsCatalogSecret", SupportsShouldProcess = true), OutputType(typeof(bool))] | ||
[Alias("Remove-AdlCatalogSecret")] | ||
[Obsolete("Catalog secrets are being deprecated in the next release. Please use Remove-AzureRmDataLakeAnalyticsCatalogCredential directly instead.")] |
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.
... in a future release
|
||
namespace Microsoft.Azure.Commands.DataLakeAnalytics | ||
{ | ||
[Cmdlet(VerbsCommon.Set, "AzureRmDataLakeAnalyticsCatalogCredential", SupportsShouldProcess = true), OutputType(typeof(USqlCredential))] |
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.
Since there are multiple parameter sets, you should specify a default parameter set name
|
||
namespace Microsoft.Azure.Commands.DataLakeStore | ||
{ | ||
[Cmdlet(VerbsCommon.Get, "AzureRmDataLakeStoreTrustedIdProvider"), OutputType(typeof(TrustedIdProvider), typeof(IList<TrustedIdProvider>))] |
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.
same comment on output types
HelpMessage = | ||
"Indicates the delete should be immediately performed with no confirmation or prompting. Use carefully." | ||
)] | ||
public SwitchParameter Force { get; set; } |
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.
(1) switch parameters should not be positional
(2) You don't need a Force in this case - there are no special conditions you are prompting for
HelpMessage = | ||
"Indicates the delete should be immediately performed with no confirmation or prompting. Use carefully." | ||
)] | ||
public SwitchParameter Force { get; set; } |
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.
switch parameters should not be positional
You don't need the Force parameter
|
||
namespace Microsoft.Azure.Commands.DataLakeStore | ||
{ | ||
[Cmdlet(VerbsCommon.Set, "AzureRmDataLakeStoreFirewallRule", SupportsShouldProcess = true), OutputType(typeof(FirewallRule))] |
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.
same comment on output type
|
||
namespace Microsoft.Azure.Commands.DataLakeStore | ||
{ | ||
[Cmdlet(VerbsCommon.Set, "AzureRmDataLakeStoreTrustedIdProvider", SupportsShouldProcess = true), OutputType(typeof(TrustedIdProvider))] |
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.
same comment on output types
Description
This brings ADL powershell up to the latest SDK version in preparation for GA. The latest SDK is still preview, however it is our first GA candidate SDK and needs to be included in powershell to bring the following features:
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.
General Guidelines
Testing Guidelines
Cmdlet Signature Guidelines
ShouldProcess
and haveSupportShouldProcess=true
specified in the cmdlet attribute. You can find more information onShouldProcess
here.OutputType
attribute if any output is produced - if the cmdlet produces no output, it should implement aPassThrough
parameter.Cmdlet Parameter Guidelines