search

tpcarman / As-Built-Report

A collection of PowerShell scripts to generate as built reports on the configuration of datacentre infrastucture in Text, XML, HTML & MS Word formats.
MIT License
88 stars 28 forks source link

Pull request for Issue #14 #15

Closed ITblacksheep closed 6 years ago

ITblacksheep commented 6 years ago

This will dynamically validate against the reports directory and also provide feedback to the user on invalid selections.

ryan-jan commented 6 years ago

Hi,

The .vscode folder is part of this repo, it contains formatting settings etc. for the project which we have agreed on

Could you remove 24c29797 from the pull request and rebase onto the current dev branch?

ryan-jan commented 6 years ago

Hi again, your recent commit c529eb6 will actually delete the .gitignore file entirely, which obviously we don't want. You would need to remove the previous two commits from your branch before we could look at merging this PR.

Thanks

ITblacksheep commented 6 years ago

I'm going to do a rebase and update the pull.

On Wed, Sep 5, 2018 at 2:39 PM Ryan Kowalewski notifications@github.com wrote:

Hi again, your recent commit c529eb6 https://github.com/tpcarman/As-Built-Report/commit/c529eb60973439fbd990cc3acfee5e276d260cb2 will actually delete the .gitignore file entirely, which obviously we don't want. You would need to remove the previous two commits from your branch before we could look at merging this PR.

Thanks

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tpcarman/As-Built-Report/pull/15#issuecomment-418836541, or mute the thread https://github.com/notifications/unsubscribe-auth/AbVbEQKStg6vw3SBmrmYV2d4PqYtD2-Dks5uYBp9gaJpZM4WZEhE .

ITblacksheep commented 6 years ago

I updated the pull. I did add 2 directories to the .gitignore. One for config/*.json so we don't pull up and commit any config files for customers and one for Output/ where is where I am dropping the output of the scripts. Also may want to include .vscode/launch.json, because when you modify debugging in vscode that gets pull in also.

ryan-jan commented 6 years ago

There are now 13 commits in this pull request which is way to many. You need to rebase and squash these into a single commit which clearly explains what you are changing.

Also why have you edited the vSphere.ps1 in this pull request? And what do your edits on this script provide?

Regarding the .gitignore file, I discussed this with Tim and Matt and we decided that it is unnecessary to add these config directories as we can just save these files elsewhere on the file system.

Please can you rework this pull request to just submit your proposed validation improvements to New-AsBuiltReport.ps1 as a single commit with a clear commit message and summary. Also, ideally commit messages should be in the present tense i.e. Update rather than Updated.

Thanks.

ITblacksheep commented 6 years ago

Sorry for the confusion. I was also working on speeding up some stuff in the vsphere report and forgot to switch branches. I reset my repo last night and was almost to the point of a pull request when my laptop died. I will get you one commit today with just the changes to the validation. I was hoping I could get it straightened out before you saw.

On Thu, Sep 6, 2018 at 1:17 AM Ryan Kowalewski notifications@github.com wrote:

There are now 13 commits in this pull request which is way to many. You need to rebase and squash these into a single commit which clearly explains what you are changing.

Also why have you edited the vSphere.ps1 in this pull request? And what do your edits on this script provide?

Regarding the .gitignore file, I discussed this with Tim and Matt and we decided that it is unnecessary to add these config directories as we can just save these files elsewhere on the file system.

Please can you rework this pull request to just submit your proposed validation improvements to New-AsBuiltReport.ps1 as a single commit with a clear commit message and summary.

Thanks.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tpcarman/As-Built-Report/pull/15#issuecomment-418967539, or mute the thread https://github.com/notifications/unsubscribe-auth/AbVbESTqCHPSvlJq-Cr4vOJoXtdmCOZ1ks5uYK_5gaJpZM4WZEhE .