Skip to content

Conversation

@blueww
Copy link
Member

@blueww blueww commented Jun 22, 2018

Description

This PR is to add the static Web data plane support.
It already pass review in https://github.com/Azure/azure-powershell-cmdlet-review-pr/issues/88

Since the XSCL still not released, this based on private XSCL and DMlib (So build won't pass). It's just for a early code review to reduce the risk for the late code review.
I will update it when XSCL released.

Checklist

@blueww
Copy link
Member Author

blueww commented Jun 26, 2018

@praries880
This PR is just for an early review before XSCL is released, to reduce the risk of the review the PR later, since we need to release it after 1 week of XSCL release.

I will definitely update it after XSCL is released.
And I don't see a conflict now.
Would you please help to review, and see it any change need for this feature?

@praries880
Copy link
Contributor

@blueww can you fix the build breaks in both the Jenkin and net core builds

@blueww
Copy link
Member Author

blueww commented Jun 27, 2018

@praries880
The build failure is caused by the XSCL /DMlib still not released, so I use the not signed private build.
I send the PR and would like to get an early code review before the XSCL real release, to reduce the late review risk.

Would you please help to review before XSCL release, and see it any change need for this feature?

@praries880
Copy link
Contributor

@blueww will review the code, will mark it as do not merge though :)

Copy link
Contributor

@praries880 praries880 left a comment

Choose a reason for hiding this comment

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

a few minor comments.. otherwise looks good to me

Copy link
Contributor

Choose a reason for hiding this comment

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

assuming this will be undone once the new SDK is released.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I will update all the reference when the XSCL is release. Currently remove the “PublicKeyToken” only to make the private XSCL work.

Copy link
Contributor

Choose a reason for hiding this comment

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

What are these about? to set them in the tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

This parameter will show the user the StaticWebsite setting after the change. So they can have a check.
This is aligned with other serverproperty cmdlets like

enable/disable-AzureStorageDeleteRetentionPolicy
Set-AzureStorageServiceLoggingProperty
Set-AzureStorageServiceMetricsProperty
Set-AzureStorageServiceProperty

Copy link
Contributor

Choose a reason for hiding this comment

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

The comment says "Overwrite the useless parameter".. these dont seem useless :)

@blueww
Copy link
Member Author

blueww commented Jun 28, 2018

@praries880

I have updated the PR to the released XSCL and private signed DMlib.
(for preview module sign pricate SDK is allowed)

I have also updated the change log and module version. (not sure if 4.4.1 is correct version)
Your command are all answered.
Would you please help to review?

Copy link
Contributor

@praries880 praries880 left a comment

Choose a reason for hiding this comment

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

Two minor nitpicks.. otherwise looks good to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

The comment says "Overwrite the useless parameter".. these dont seem useless :)

[Parameter(Mandatory = false)]
public SwitchParameter PassThru { get; set; }

// Overwrite the useless parameter
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these really useless?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, these are useless. These parameter are for the long running blob/file content transfer cmdlets. But for this cmdlet, it's just a property setting, and can finish very quickly, don't need set special timeout and concurrent.

@blueww
Copy link
Member Author

blueww commented Jul 2, 2018

@praries880 praries880 merged commit bfb29d3 into Azure:AzureRM.Storage.Location Jul 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants