-
Notifications
You must be signed in to change notification settings - Fork 38
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 Windows 7/Server 2008 Support #345
base: main
Are you sure you want to change the base?
Conversation
@PlagueHO @dscottraynsford - Any chance we can get this merged in? |
Hi @bruckect - sorry about the delay. Can you rebase for me to resolve the conflicts and I'll get this through once the build passes. |
I'm not sure how to rebase and keep the PR intact. It looks like you can rebase when you accept the PR. Docs |
Hi @bruckect - I should have been more specific 😁 I meant can you fix the merge conflicts above. The recommended way of doing that is to rebase your branch onto main. https://guides.codechewing.com/git/rebase-merge-conflicts But you can fix the merge conflicts by just merging upstream main into your branch and then manually fixing the conflicts. |
@PlagueHO @dscottraynsford I think I did it, forgive my ignorance with Git, still learning. Please let me know if I need to do something different. |
@PlagueHO @dscottraynsford - Is the PR in the right state to be merged? I'd like to get this pushed through so that I can address some additional functionality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 5 files at r1, 4 of 8 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @bruckect)
source/Private/Get-LabCertificatePsFileContent.ps1, line 83 at r2 (raw file):
} } if ((Get-WmiObject -Class Win32_OperatingSystem).Version -eq '6.1.7601') {
What happens for an OS that is not quite at the same version? E.g. 6.1.9000, 6.1.6090?
What you could do is change the comparison to be:
$osVersion = [Version] (Get-WmiObject -Class Win32_OperatingSystem).Version
if ($osVersion.Major -eq 6 -and $osVersion.Minor -eq 1) {
source/Private/Get-LabCertificatePsFileContent.ps1, line 84 at r2 (raw file):
} if ((Get-WmiObject -Class Win32_OperatingSystem).Version -eq '6.1.7601') { `$content = @(
How come the backtick here?
source/Private/Get-LabCertificatePsFileContent.ps1, line 89 at r2 (raw file):
'-----END CERTIFICATE-----' ) `$content | Out-File -FilePath `"`$(`$ENV:SystemRoot)\$Script:DSCEncryptionCert`" -Encoding ascii
How come the backtick here?
source/Private/Get-LabUnattendFileContent.ps1, line 136 at r2 (raw file):
"@ } # If if (($VM.Name -like '*7*' -or $VM.Name -like '*2008*') -and $VM.Name -notlike '*10*') {
This check might cause problems - for example it would be quite possible for a VM name to include '7' or '10' (I have a couple of labs that do have both). Is there any other way we could detect the OS is 2008? Maybe we need a new flag in the Lab XML to indicate the OS that is being installed?
source/Private/Set-LabDSC.ps1, line 74 at r2 (raw file):
$macAddress = $netAdapter.MacAddress $mac = $macAddress.insert(2,":").insert(5,":").insert(8,":").insert(11,":").insert(14,":")
Can you move this line into the if block below, because technical this conversion isn't required unless on WS2008.
source/Private/Set-LabDSC.ps1, line 88 at r2 (raw file):
$dscStartPs += @" if ((Get-WmiObject -Class Win32_OperatingSystem).Version -eq '6.1.7601') {
See previous comment about detecting OS version.
source/Private/Set-LabDSC.ps1, line 92 at r2 (raw file):
} else { Get-NetAdapter `` | Where-Object { `$_.MacAddress.Replace('-','') -eq '$macAddress' } ``
Style nitpick, can you put the | on the end of the previous line - then the back tick is not needed. Not sure why I didn't do this :)
E.g.
Get-NetAdapter |
Where-Object { `$_.MacAddress.Replace('-','') -eq '$macAddress' } |
Rename-NetAdapter -NewName '$($adapter)'
Hello @PlagueHO - below are some changes that support Windows 7/Server 2008, fixed some minor issues that I introduced in the last round of PR's and added support for Server 2019 Hyper-V configuration version 9.0.
Bug Fixes
Improvements / Enhancements
This change is