Skip to content
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

Adding xNetAdapterRDMA #165

Merged
merged 7 commits into from Dec 13, 2016
Merged

Adding xNetAdapterRDMA #165

merged 7 commits into from Dec 13, 2016

Conversation

rchaganti
Copy link
Contributor

@rchaganti rchaganti commented Dec 7, 2016

Adding xNetAdapterRDMA resource for configuring RDMA on network adapters.


This change is Reviewable

@PlagueHO PlagueHO added the needs review The pull request needs a code review. label Dec 7, 2016
@PlagueHO
Copy link
Member

PlagueHO commented Dec 7, 2016

Thanks for resubmitting this @rchaganti - really appreciate all your work on this! Just one tiny little change if you don't mind? Then it :LGTM:


Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


DSCResources/MSFT_xNetAdapterRDMA/MSFT_xNetAdapterRDMA.schema.mof, line 4 at r1 (raw file):

class MSFT_xNetAdapterRDMA : OMI_BaseResource
{
    [Key] String Name;

Sorry to be a complete pain, but could you add the Description (from the Readme.md if you like) to the MOF files here? I'm eventually hoping we can implement the code that xSharepoint uses which uses the description from the MOF to generate the documentation.


DSCResources/MSFT_xNetAdapterRDMA/MSFT_xNetAdapterRDMA.schema.mof, line 5 at r1 (raw file):

{
    [Key] String Name;
    [Write] Boolean Enabled;

Ditto.


Comments from Reviewable

@PlagueHO PlagueHO added waiting for author response The pull request is waiting for the author to respond to comments in the pull request. and removed needs review The pull request needs a code review. labels Dec 7, 2016
@rchaganti
Copy link
Contributor Author

Review status: 8 of 9 files reviewed at latest revision, 2 unresolved discussions.


DSCResources/MSFT_xNetAdapterRDMA/MSFT_xNetAdapterRDMA.schema.mof, line 4 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Sorry to be a complete pain, but could you add the Description (from the Readme.md if you like) to the MOF files here? I'm eventually hoping we can implement the code that xSharepoint uses which uses the description from the MOF to generate the documentation.

Complete


DSCResources/MSFT_xNetAdapterRDMA/MSFT_xNetAdapterRDMA.schema.mof, line 5 at r1 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Ditto.

Complete


Comments from Reviewable

@rchaganti
Copy link
Contributor Author

Done!

Last build failed on other DSC resources.
@rchaganti
Copy link
Contributor Author

@PlagueHO do you know why the build is failing? Especially for xFirewall?

@PlagueHO
Copy link
Member

PlagueHO commented Dec 7, 2016

@rchaganti - Ok, I know why the integration tests are failing. A change was made to the DSCResources.Tests a few weeks back which caused this. Basically the tests were changed over to use the Pester $TestDrive as working storage.

The solution is to change the line:

& "$($script:DSCResourceName)_Config" -OutputPath $TestEnvironment.WorkingFolder

in each MSFT_*.Integration.Tests.ps1 file to read:

& "$($script:DSCResourceName)_Config" -OutputPath $TestDrive

Would you mind making this change for us?

The problem with xFirewall is actually caused by a bug in DSCResource.Tests. I'll actually submit a PR to DSCResource.Tests to fix this, but to work around the problem:

Change the filename of the xFirewall schema file MSFT_xFirewall.Schema.mof to be use a lower case .Schema (the capital S is the problem). E.g. rename it to MSFT_xFirewall.schema.mof

Thanks again!

@PlagueHO
Copy link
Member

PlagueHO commented Dec 7, 2016

@rchaganti - I've submitted a PR to DSCResources.Tests to fix the .schema issue:
PowerShell/DscResource.Tests#82

@rchaganti
Copy link
Contributor Author

I updated the integration test file based on your feedback.

@PlagueHO
Copy link
Member

PlagueHO commented Dec 7, 2016

@rchaganti - hmm. Something else must be going on here. I'll take a look through this tonight and advise exactly what needs to be fixed. Sorry about all this.

The fix for the .Schema has gone through in DSCResources.Tests so that should be resolved.

@PlagueHO
Copy link
Member

PlagueHO commented Dec 7, 2016

Hi @rchaganti - ok. Found the problem. There was another change to DSCResource.Tests that effected this. Maybe the best thing for me to do is submit a new PR with all the fixes to support the new tests in place and get that merged.

You can then merge that with your changes. Saves me having to explain all the fixes here. What do you reckon?

@PlagueHO
Copy link
Member

PlagueHO commented Dec 7, 2016

Reviewed 1 of 1 files at r2, 1 of 1 files at r5.
Review status: 8 of 9 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Tests/Integration/MSFT_xNetAdapterRDMA.Integration.Tests.ps1, line 36 at r5 (raw file):

            {
                & "$($script:DSCResourceName)_Config" -OutputPath $TestDrive
                Start-DscConfiguration -Path $TestEnvironment.WorkingFolder `

My bad - you'll also need to change $TestEnvironment.Working folder here to $TestDrive.


Comments from Reviewable

@rchaganti
Copy link
Contributor Author

Review status: 8 of 9 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Tests/Integration/MSFT_xNetAdapterRDMA.Integration.Tests.ps1, line 36 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

My bad - you'll also need to change $TestEnvironment.Working folder here to $TestDrive.

Done


Comments from Reviewable

@rchaganti
Copy link
Contributor Author

readme conflict again!?

@PlagueHO
Copy link
Member

@rchaganti - unfortunately, yes. This is because PR #168 had to go in so that your tests would not fail. PR #168 included a new entry in the Readme.md to identify the change. But the conflict should be easy to resolve I think. Sorry about that.

@PlagueHO
Copy link
Member

@rchaganti - I didn't realize there was a new GitHub feature that allowed me to resolve the conflict for you. So it is resolved.

@PlagueHO
Copy link
Member

Reviewed 1 of 1 files at r6, 1 of 1 files at r7.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@PlagueHO PlagueHO merged commit ef3d914 into dsccommunity:dev Dec 13, 2016
@vors vors removed the waiting for author response The pull request is waiting for the author to respond to comments in the pull request. label Dec 13, 2016
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.

None yet

4 participants