Update to R4.5 RP standard (#22) #9680

leonardbf · 2019-07-18T13:22:12Z

Addition of protocolTypes
Internal ticket NFSAAS-2898

Description

Checklist

I have read the Submitting Changes section of CONTRIBUTING.md
The title of the PR is clear and informative
The appropriate change log has been updated
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:
- the changes have gone through a cmdlet design review (Microsoft internal partners only)
- the cmdlet markdown files were generated using the platyPS module

leonardbf · 2019-07-18T13:53:45Z

Static analysis failures but these don't look related to the changes being made. Please advise.

niander · 2019-07-19T00:00:57Z

src/NetAppFiles/NetAppFiles/Volume/NewNetAppFilesVolume.cs

@@ -83,7 +84,7 @@ public class NewAzureRmNetAppFilesVolume : AzureNetAppFilesCmdletBase
            Mandatory = true,
            HelpMessage = "The maximum storage quota allowed for a file system in bytes")]
        [ValidateNotNullOrEmpty]
-        public long? UsageThreshold { get; set; }
+        public long UsageThreshold { get; set; }


This is one of the places causing the breaking change issue

niander · 2019-07-19T00:01:11Z

src/NetAppFiles/NetAppFiles/Pool/NewNetAppFilesPool.cs

@@ -71,7 +71,7 @@ public class NewAzureRmNetAppFilesPool : AzureNetAppFilesCmdletBase
            Mandatory = true,
            HelpMessage = "The size of the ANF pool")]
        [ValidateNotNullOrEmpty]
-        public long? PoolSize { get; set; }
+        public long PoolSize { get; set; }


This is one of the places causing the breaking change issue

niander · 2019-07-19T00:02:27Z

@leonardbf I made comments on where is causing the breaking change issue. Please take a look. You were removing the nullable property of these type (which is the ? suffix for the type names)

leonardbf · 2019-07-19T09:01:12Z

@leonardbf I made comments on where is causing the breaking change issue. Please take a look. You were removing the nullable property of these type (which is the ? suffix for the type names)

@niander the nullable property in the generated code changed because the swagger changed such that these became required fields. I therefore matched this in the cmdlet and actually this was necessary to resolve a build error in assigning the non-nullable to nullable in the cmdlet body. I suppose I could provide a default but this is contrary to what expected. What's the normal solution to resolve this. Also, can you confirm, is this the sole cause of the failing checks?

niander · 2019-07-19T16:51:43Z

@leonardbf Hi! I am sorry but I forgot that NetApp is still in preview. Thus, its is ok to have this breaking change. The only thing we need here is to suppress it. Check our docs here to see how to do it.

I am not sure if this is the sole cause of all failing checks but according to the BreakingChangeIssues.csv those two are the only static analysis breaking change issues.

niander

just a small comment and I think after suppressing the breaking changes and passing CI it is good to go

niander · 2019-07-19T16:55:37Z

src/NetAppFiles/NetAppFiles.Test/Properties/AssemblyInfo.cs

@@ -45,6 +45,6 @@
 //
 // You can specify all the values or you can default the Build and Revision Numbers 
 // by using the '*' as shown below:
-[assembly: AssemblyVersion("0.9.0")]
-[assembly: AssemblyFileVersion("0.9.0")]
+[assembly: AssemblyVersion("1.1.0")]


You don't need to change assembly versions for test projects

ok, I will revert this

markcowl · 2019-07-22T20:56:46Z

@leonardbf Ping waiting for your changes to fix the anlysis breaks, per Niander's comments

leonardbf · 2019-07-23T10:54:28Z

@leonardbf Hi! I am sorry but I forgot that NetApp is still in preview. Thus, its is ok to have this breaking change. The only thing we need here is to suppress it. Check our docs here to see how to do it.

I am not sure if this is the sole cause of all failing checks but according to the BreakingChangeIssues.csv those two are the only static analysis breaking change issues.

@niander or @markcowl ok, thanks. I have looked at the static analysis page referenced above and see that I need to add the breaking changes file to the exceptions area. However, I'm not sure where I find the original BreakingChangeIssues.csv. It says it is under the Jenkin's build artifacts. Can you give me the link to that please. Also, I'd like to be sure about the preview status. The ANF service went GA a few weeks ago so does that not also mean that the PowerShell cmdlets are actually now GA?

leonardbf · 2019-07-26T09:00:14Z

@niander assistance with the couple of points in my previous comment would be appreciated. Many thanks.

niander

Just forget to add a comment and please change the changelog.md file

niander · 2019-07-29T17:58:37Z

src/NetAppFiles/NetAppFiles/Properties/AssemblyInfo.cs

@@ -24,5 +24,5 @@
 [assembly: ComVisible(false)]
 [assembly: CLSCompliant(false)]
 [assembly: Guid("42656543-77AD-4968-BA4B-BE0778705625")]
-[assembly: AssemblyVersion("0.1.1")]
-[assembly: AssemblyFileVersion("0.1.1")]
+[assembly: AssemblyVersion("1.1.0")]


You don't need to change this too

niander · 2019-07-29T18:00:32Z

@leonardbf Hi. You can find the BreakingChangeIssues.csv file in your computer when you build your project and run the static analysis. It is the same one generated by the CI (azure devops, since we are not using jenkins anymore. Sorry about that). About the PowerShell module going from Preview to Release it should happen after we merge these changes and release a new version of it. After that we can talk about going to Release and adding the module to Az

leonardbf · 2019-08-01T14:33:37Z

@niander I updated the PR with the BreakingChangeIssues.csv file just to check the validation but the pipeline now seems to be stuck. I don't see any links to the process to try and restart.

cormacpayne · 2019-08-01T16:24:36Z

/azp run

azure-pipelines · 2019-08-01T16:24:41Z

Pull request contains merge conflicts.

leonardbf · 2019-08-02T22:10:25Z

Replaced with #9762

azuresdkci added the needs-review label Jul 18, 2019

niander reviewed Jul 19, 2019

View reviewed changes

niander assigned leonardbf Jul 19, 2019

niander added needs-revision and removed needs-review labels Jul 19, 2019

niander suggested changes Jul 19, 2019

View reviewed changes

cormacpayne assigned niander and unassigned leonardbf Jul 29, 2019

cormacpayne added needs-review and removed needs-revision labels Jul 29, 2019

niander suggested changes Jul 29, 2019

View reviewed changes

niander added needs-revision and removed needs-review labels Jul 29, 2019

niander assigned leonardbf and unassigned niander Jul 29, 2019

leonardbf closed this Aug 2, 2019

leonardbf force-pushed the master branch from 8e279e0 to 7f036ca Compare August 2, 2019 21:30

leonardbf mentioned this pull request Aug 2, 2019

Update to R4.5 RP standard (#24) #9762

Merged

8 tasks

Update to R4.5 RP standard (#22) #9680

Update to R4.5 RP standard (#22) #9680

Uh oh!

Conversation

leonardbf commented Jul 18, 2019

Description

Checklist

Uh oh!

leonardbf commented Jul 18, 2019

Uh oh!

niander Jul 19, 2019

Choose a reason for hiding this comment

Uh oh!

niander Jul 19, 2019

Choose a reason for hiding this comment

Uh oh!

niander commented Jul 19, 2019

Uh oh!

leonardbf commented Jul 19, 2019

Uh oh!

niander commented Jul 19, 2019

Uh oh!

niander left a comment

Choose a reason for hiding this comment

Uh oh!

niander Jul 19, 2019

Choose a reason for hiding this comment

Uh oh!

leonardbf Jul 23, 2019

Choose a reason for hiding this comment

Uh oh!

markcowl commented Jul 22, 2019

Uh oh!

leonardbf commented Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leonardbf commented Jul 26, 2019

Uh oh!

niander left a comment

Choose a reason for hiding this comment

Uh oh!

niander Jul 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niander commented Jul 29, 2019

Uh oh!

leonardbf commented Aug 1, 2019

Uh oh!

cormacpayne commented Aug 1, 2019

Uh oh!

azure-pipelines bot commented Aug 1, 2019

Uh oh!

leonardbf commented Aug 2, 2019

Uh oh!

Uh oh!

leonardbf commented Jul 23, 2019 •

edited

Loading

niander Jul 29, 2019 •

edited

Loading