Skip to content

[HDInsight] Add three parameters to support relay outbound and private link and custom Ambari DB#13413

Merged
isra-fel merged 4 commits intoAzure:masterfrom
aim-for-better:SupportRelayOutboundAndPrivateLink
Nov 10, 2020
Merged

[HDInsight] Add three parameters to support relay outbound and private link and custom Ambari DB#13413
isra-fel merged 4 commits intoAzure:masterfrom
aim-for-better:SupportRelayOutboundAndPrivateLink

Conversation

@aim-for-better
Copy link
Copy Markdown
Member

@aim-for-better aim-for-better commented Nov 4, 2020 •
edited
Loading

Description

For the cmdlet New-AzHDInsightCluster

  • Add two parameters ResourceProviderConnection and PrivateLink to support Relay and Private Link
  • Add one parameters AmbariDatabase to support custom Ambari DB

For the cmdlet Add-AzHDInsightMetastore:

  • Add the accept value "AmbariDatabase" for the paramter -MetastoreType

Checklist

  • I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:
    • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
    • the markdown help files have been regenerated using the commands listed here

@aim-for-better aim-for-better changed the title [HDInsight] Support Relay Outbound and Private Link [HDInsight] Add two parameters to support relay outbound and private link Nov 4, 2020
@aim-for-better aim-for-better force-pushed the SupportRelayOutboundAndPrivateLink branch from 741e7a7 to d08ff19 Compare November 4, 2020 05:41
@aim-for-better
Copy link
Copy Markdown
Member Author

aim-for-better commented Nov 4, 2020

The cmdlet Restart-AzHDInsightHost doesn't use the property NetworkSettings, but I still got the static analysis error,
I think this is false alarm, because our code doesn't use this property. So I have to suppress this.

"AssemblyFileName","ClassName","Target","Severity","ProblemId","Description","Remediation"
"Microsoft.Azure.PowerShell.Cmdlets.HDInsight.dll","Microsoft.Azure.Commands.HDInsight.RestartAzureHDInsightHostCommand","Restart-AzHDInsightHost","0","3010","The property 'NetworkSettings' of type 'Microsoft.Azure.Management.HDInsight.Models.ClusterGetProperties' has been removed.","Add the property 'NetworkSettings' back to type 'Microsoft.Azure.Management.HDInsight.Models.ClusterGetProperties'."

@aim-for-better aim-for-better added this to the S178 (2020-11-17) milestone Nov 4, 2020
@aim-for-better aim-for-better requested review from dingmeng-xue, isra-fel and wyunchi-ms and removed request for dingmeng-xue November 4, 2020 05:57
@isra-fel isra-fel self-assigned this Nov 4, 2020
@aim-for-better aim-for-better requested review from MiYanni and dingmeng-xue and removed request for MiYanni and dingmeng-xue November 4, 2020 11:36
@aim-for-better aim-for-better changed the title [HDInsight] Add two parameters to support relay outbound and private link [HDInsight] Add three parameters to support relay outbound and private link and custom Ambari DB Nov 5, 2020
@aim-for-better aim-for-better force-pushed the SupportRelayOutboundAndPrivateLink branch from 0396b9d to d20e5ac Compare November 5, 2020 06:30
@aim-for-better
Copy link
Copy Markdown
Member Author

aim-for-better commented Nov 5, 2020

Hi @isra-fel Could you please help review the PR? Thank you very much~

isra-fel
isra-fel reviewed Nov 5, 2020
View reviewed changes
@isra-fel
Copy link
Copy Markdown
Member

isra-fel commented Nov 6, 2020 •
edited
Loading

The cmdlet Restart-AzHDInsightHost doesn't use the property NetworkSettings, but I still got the static analysis error,
I think this is false alarm, because our code doesn't use this property. So I have to suppress this.

"AssemblyFileName","ClassName","Target","Severity","ProblemId","Description","Remediation"
"Microsoft.Azure.PowerShell.Cmdlets.HDInsight.dll","Microsoft.Azure.Commands.HDInsight.RestartAzureHDInsightHostCommand","Restart-AzHDInsightHost","0","3010","The property 'NetworkSettings' of type 'Microsoft.Azure.Management.HDInsight.Models.ClusterGetProperties' has been removed.","Add the property 'NetworkSettings' back to type 'Microsoft.Azure.Management.HDInsight.Models.ClusterGetProperties'."

I understand what you mean. I think the root cause is the OutputType of Restart-AzHDInsightHost was set to Cluster (see https://github.com/Azure/azure-powershell/blob/master/src/HDInsight/HDInsight/ManagementCommands/RestartAzureHDInsightHost.cs#L28 ) but it actually returns a bool. Our static analysis tool was based on OutputType so it thought that type has changed.

So I think the right thing to do is to fix the output type. Could you help fix it in this PR? Thanks

@aim-for-better
Copy link
Copy Markdown
Member Author

aim-for-better commented Nov 6, 2020

has

The cmdlet Restart-AzHDInsightHost doesn't use the property NetworkSettings, but I still got the static analysis error,
I think this is false alarm, because our code doesn't use this property. So I have to suppress this.
"AssemblyFileName","ClassName","Target","Severity","ProblemId","Description","Remediation"
"Microsoft.Azure.PowerShell.Cmdlets.HDInsight.dll","Microsoft.Azure.Commands.HDInsight.RestartAzureHDInsightHostCommand","Restart-AzHDInsightHost","0","3010","The property 'NetworkSettings' of type 'Microsoft.Azure.Management.HDInsight.Models.ClusterGetProperties' has been removed.","Add the property 'NetworkSettings' back to type 'Microsoft.Azure.Management.HDInsight.Models.ClusterGetProperties'."

I understand what you mean. I think the root cause is the OutputType of Restart-AzHDInsightHost was set to Cluster (see https://github.com/Azure/azure-powershell/blob/master/src/HDInsight/HDInsight/ManagementCommands/RestartAzureHDInsightHost.cs#L28 ) but it actually returns a bool. Our static analysis tool was based on OutputType so it thought that type has changed.

So I think the right thing to do is to fix the output type. Could you help fix it in this PR? Thanks

Hi @isra-fel , Got it, Thanks, I will fix the output type.

@isra-fel
Copy link
Copy Markdown
Member

isra-fel commented Nov 6, 2020

A reminder: now that you will get a static analysis warning about changing the output type of Restart-AzHDInsightHost which needs to be suppressed.

@aim-for-better aim-for-better force-pushed the SupportRelayOutboundAndPrivateLink branch from f2b1e14 to 82034ab Compare November 6, 2020 05:27
@aim-for-better
Copy link
Copy Markdown
Member Author

aim-for-better commented Nov 6, 2020 •
edited
Loading

A reminder: now that you will get a static analysis warning about changing the output type of Restart-AzHDInsightHost which needs to be suppressed.

Hi @isra-fel , Thanks for reminder, in fact I got two static analysis error, besides the error you mentioned
there is a new NetworkSettings property has been removed issue for Set-AzHDInsightClusterDiskEncryptionKey 😂,
I suppress both of them.
Please see the previous static analysis error
https://dev.azure.com/azure-sdk/public/_build/results?buildId=608528&view=logs&jobId=99d77eec-fdb1-5e11-3259-110c26331ac6&j=99d77eec-fdb1-5e11-3259-110c26331ac6&t=d24a834f-31ee-5ffa-ba54-b348410dd76a

isra-fel
isra-fel previously approved these changes Nov 6, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@isra-fel isra-fel left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks

@isra-fel
Copy link
Copy Markdown
Member

isra-fel commented Nov 9, 2020

/azp run azure-powershell - powershell-core

@azure-pipelines
Copy link
Copy Markdown
Contributor

azure-pipelines bot commented Nov 9, 2020

Azure Pipelines successfully started running 1 pipeline(s).

@aim-for-better
Copy link
Copy Markdown
Member Author

aim-for-better commented Nov 9, 2020

/azp run azure-powershell - powershell-core

Hi @isra-fel Thanks

@isra-fel
Copy link
Copy Markdown
Member

isra-fel commented Nov 9, 2020

/azp run azure-powershell - powershell-core

@azure-pipelines
Copy link
Copy Markdown
Contributor

azure-pipelines bot commented Nov 9, 2020

Azure Pipelines successfully started running 1 pipeline(s).

@aim-for-better
Copy link
Copy Markdown
Member Author

aim-for-better commented Nov 9, 2020

Hi @isra-fel , The CI still failed. The error is not related with specific service, I think maybe it is common issue. Please see https://dev.azure.com/azure-sdk/public/_build/results?buildId=610796&view=logs&j=ad07b744-5a1a-5d54-7693-fcb02e666633&t=b0841cae-a6b1-5a5c-4ebd-3cc6b3d3163d

@aim-for-better
Copy link
Copy Markdown
Member Author

aim-for-better commented Nov 9, 2020

/azp run azure-powershell - powershell-core

@azure-pipelines
Copy link
Copy Markdown
Contributor

azure-pipelines bot commented Nov 9, 2020

Azure Pipelines successfully started running 1 pipeline(s).

@aim-for-better
Copy link
Copy Markdown
Member Author

aim-for-better commented Nov 9, 2020

/azp run azure-powershell - powershell-core (Test linux)

@azure-pipelines
Copy link
Copy Markdown
Contributor

azure-pipelines bot commented Nov 9, 2020

No pipelines are associated with this pull request.

@aim-for-better
Copy link
Copy Markdown
Member Author

aim-for-better commented Nov 9, 2020 •
edited
Loading

PowerShell core phase still failed
image

@aim-for-better aim-for-better force-pushed the SupportRelayOutboundAndPrivateLink branch from a5c3828 to 77e4232 Compare November 9, 2020 07:38
@aim-for-better
Copy link
Copy Markdown
Member Author

aim-for-better commented Nov 9, 2020 •
edited
Loading

The CI failed for ##[error]Artifact test-linux already exists for build 610796 more than one time. So I just generate a new commit id to rerun CI totally.

@isra-fel
Copy link
Copy Markdown
Member

isra-fel commented Nov 9, 2020

/azp run azure-powershell - powershell-core

@azure-pipelines
Copy link
Copy Markdown
Contributor

azure-pipelines bot commented Nov 9, 2020

Azure Pipelines successfully started running 1 pipeline(s).

@isra-fel isra-fel merged commit 6cf9281 into Azure:master Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@isra-fel isra-fel isra-fel approved these changes

@wyunchi-ms wyunchi-ms Awaiting requested review from wyunchi-ms

Assignees

@isra-fel isra-fel

Labels

Waiting for CI :shipit:

Projects

None yet

Milestone

S178 (2020-11-17)

Development

Successfully merging this pull request may close these issues.

3 participants

@aim-for-better @isra-fel @azuresdkci