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 xHostsFile resource to close issue #8 #93

Closed
wants to merge 7 commits into from
Closed

Adding xHostsFile resource to close issue #8 #93

wants to merge 7 commits into from

Conversation

rchaganti
Copy link
Contributor

No description provided.

@msftclas
Copy link

msftclas commented Feb 5, 2016

Hi @rchaganti, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

#endregion
}
finally
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably backup/restore the host file to clear out any modifications we made to it during the integration tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! I am copying the file to $env:Temp and restoring it from there.

Updates to address feedback from @tysonjhayes.
IPAddress = $IPAddress
}

Write-Verbose $localizedData.checkingHostsFileEntry
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hate to be a pain, but are we able to eliminate the use of any positional parameters? In general it is recommended against for production code (https://blogs.technet.microsoft.com/heyscriptingguy/2012/04/22/the-problem-with-powershell-positional-parameters/).

e.g. Change

Write-Verbose $localizedData.checkingHostsFileEntry

to:

Write-Verbose -Message $localizedData.checkingHostsFileEntry

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:) No major problem there. I should have taken care of that. Will do now! :)

@PlagueHO
Copy link
Member

PlagueHO commented Feb 8, 2016

Nice work BTW - sorry to be nit picky about style. Thanks for working on this 👍

@rchaganti
Copy link
Contributor Author

Do you know why the last build failed? It took more than 7hrs to fail and I don't see anything in the logs. Could be a build system issue?

@PlagueHO
Copy link
Member

PlagueHO commented Feb 8, 2016

I've had this problem before (and so have others as well). It usually seems to occur during the PSScriptAnalyzer checks. No idea why it happens though.

I usually just commit a "dummy" change to trigger a rebuild and it works fine. Just add a space to the end of the one of the "Changes" lines in the # Unreleased section of the readme.md or something like that and commit it.

@rchaganti
Copy link
Contributor Author

@PlagueHO that did the trick! :)

@TravisEz13
Copy link
Contributor

Just a reminder - Please resolve merge conflicts. For more information see GitHowTo - 30. Resolving Conflicts
[Instructions to setup BC4](http://www.scootersoftware.com/support.php?zz=kb_vcs#gitwindows

@rchaganti
Copy link
Contributor Author

I messed up my local repo while trying to resolve merge conflicts! A bit more of Github / Git expertise would have been helpful. I moved the HEAD to a well-known commit in my local repo however the pull request commit to the PowerShell/Dev is one commit ahead. I am unable to do a force merge.

What are the other options?

@TravisEz13
Copy link
Contributor

@rchaganti you could delete you local branch and fetch and checkout the branch from github. This should get you back to the state you PR is in.

Here are some notes I usually point to on how to merge:
GitHowTo - 30. Resolving Conflicts
Instructions to setup BC4, my favorite merge tool

@rchaganti
Copy link
Contributor Author

Whatever I did! This thing resulted in a new PR! :( but a clean one.

@TravisEz13
Copy link
Contributor

Looks like this PR is replaced by:
#98

@TravisEz13
Copy link
Contributor

Addressed in PR #98

@TravisEz13 TravisEz13 closed this Feb 15, 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.

5 participants