-
Notifications
You must be signed in to change notification settings - Fork 0
[WIP] Az.Ssh #2
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
base: myTrack2
Are you sure you want to change the base?
[WIP] Az.Ssh #2
Conversation
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.
Some initial comments
//As Compute module must not reference to storage SDK, the types of parameters and return values of | ||
//public APIs within this project should not expose any types defined in storage SDK. | ||
|
||
public class ComputeClient |
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.
Usually, editors are set to convert tab characters into spaces. Not sure if that is true for Az cmdlets.
public class ComputeClient | |
public class ComputeClient |
# PowerShellHostVersion = '' | ||
|
||
# Minimum version of Microsoft .NET Framework required by this module. This prerequisite is valid for the PowerShell Desktop edition only. | ||
DotNetFrameworkVersion = '4.7.2' |
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.
We normally target 4.6.1
for WindowsPowerShell 5.1 compatibility.
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 will confirm with Azure PowerShell if that is the case for their modules as well. I set it to 4.7.2 because other Az PowerShell modules all do the same.
|
||
namespace Microsoft.Azure.PowerShell.Cmdlets.Ssh.Common | ||
{ | ||
class SshResourceIdCompleterAttribute : ArgumentCompleterAttribute |
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 PowerShell code base, we are always explicit about type accessiblity.
class SshResourceIdCompleterAttribute : ArgumentCompleterAttribute | |
private class SshResourceIdCompleterAttribute : ArgumentCompleterAttribute |
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.
When I do this change I get an error:
"Elements defined in a namespace cannot be explicitly declared as private, protected, protected internal, or private protected."
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.
Sorry, I guess this would be internal
. I assume it is not a type intended for public use?
DefaultParameterSetName = InteractiveParameterSet)] | ||
[OutputType(typeof(bool))] | ||
[Alias("Enter-AzArcServer")] | ||
public class EnterAzVMCommand : SshBaseCmdlet |
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.
public class EnterAzVMCommand : SshBaseCmdlet | |
public sealed class EnterAzVMCommand : SshBaseCmdlet |
[Cmdlet("Export", | ||
ResourceManager.Common.AzureRMConstants.AzureRMPrefix + "SshConfig")] | ||
[OutputType(typeof(PSSshConfigEntry))] | ||
public class ExportAzSshConfig : SshBaseCmdlet |
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.
public class ExportAzSshConfig : SshBaseCmdlet | |
public sealed class ExportAzSshConfig : SshBaseCmdlet |
…d an Azure Resource Id Parser
Description
Checklist
CONTRIBUTING.md
and reviewed the following information:generation
branch.ChangeLog.md
file(s) appropriatelyChangeLog.md
file can be found atsrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
## Upcoming Release
header in the past tense. Add changelog in description section if PR goes intogeneration
branch.ChangeLog.md
if no new release is required, such as fixing test case only.