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

Add auto support & fix appveyor build #16

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

kewalaka
Copy link

@kewalaka kewalaka commented Jun 11, 2017

This addresses issues #14 and #15


This change is Reviewable

@dsccommunity dsccommunity deleted a comment from msftclas Sep 27, 2017
@johlju johlju added the needs review The pull request needs a code review. label May 16, 2018
@johlju
Copy link
Member

johlju commented May 23, 2018

@kewalaka Sorry that it took so long for someone to review this. If I review this one, do you have time to work on it?

@johlju johlju 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 May 23, 2018
@kewalaka
Copy link
Author

hi @johlju I want to say yes but honestly would struggle. i know this resource needs a lot of work to get it to standard based on what ive seen elsewhere and it's one I have little use for. xcopy so much easier when it's practical!

@johlju
Copy link
Member

johlju commented May 29, 2018

The biggest issue is to get tests for this. It looks like we could build an integration tests for this, but this resource module does not even have unit tests, so this could should at least be tested with unit tests. But would love to have an integration tests for the composite resource.
So it a bit of a hill to climb over before I feel we can merge this PR.

It sounds like you won't have the time to put in the work to continue this PR, so I will label this as abandoned. If you do want to continue working on this, just say so I and I will relabel it as 'waiting for code fix'.

Note for myself: I have not reviewed the code yet. Just some of the simpler changes.


Review status: 3 of 7 files reviewed at latest revision, all discussions resolved.


ResourceSetHelper.psm1, line 1 at r1 (raw file):

# This module should not write any verbose or error messages unless a localization file for it is added

We should add tests for this, could the tests in PSDscResource be copied in?
https://github.com/PowerShell/PSDscResources/blob/15b40ddabdf6a46e40fc17d0d07535cf12580c89/Tests/Unit/ResourceSetHelper.Tests.ps1


DSCResources/xWebDeploy/xWebDeploy.Schema.psm1, line 2 at r1 (raw file):

# Import ResourceSetHelper for New-ResourceSetConfigurationScriptBlock

We should have an integration test that test this - maybe xWebAdministration could be an inspiration for how to accomplish this.


Comments from Reviewable

@johlju johlju added abandoned The pull request has been abandoned. and removed waiting for author response The pull request is waiting for the author to respond to comments in the pull request. labels May 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned The pull request has been abandoned.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants