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

(maint) convert install_puppet.bat to CRLF #183

Merged

Conversation

ferventcoder
Copy link
Contributor

Use CRLF instead of just LF for the template. It is not
known if using LF will cause issues with Batch files in
any versions of Windows. It is thought that running a
batch with LF should work, but it is possible that there
may be some issues somewhere. It's typically better to be
on the safe side and ensure the file is CRLF. Newer versions
of Puppet should leave the EOL format the same.

Use CRLF instead of just LF for the template. It is not
known if using LF will cause issues with Batch files in
any versions of Windows. It is thought that running a
batch with LF should work, but it is possible that there
may be some issues somewhere. It's typically better to be
on the safe side and ensure the file is CRLF. Newer versions
of Puppet should leave the EOL format the same.
@ferventcoder
Copy link
Contributor Author

ferventcoder commented Oct 24, 2016

@Iristyle @joshcooper @glennsarti @MikaelSmith let me know if there is already a reason we went with LF.

@MikaelSmith
Copy link
Contributor

I'm not aware of one.

@puppetcla
Copy link

CLA signed by all contributors.

@joshcooper
Copy link
Contributor

I don't think there are any issues running with LF on windows. Changing to CRLF will make it easier to load the resulting bat file using windows notepad. That said, there could be other parts of the CI pipelines that don't preserve CRLF when doing a git clone. IOW, even if you change this file, the resulting file on disk after MSI install may have lost the CR along the way, depending on the git config of the builder host that runs vanagon.

@ferventcoder
Copy link
Contributor Author

Fair enough. I'm happy with this though. 👍

@MikaelSmith MikaelSmith merged commit 549706a into puppetlabs:master Oct 25, 2016
@ferventcoder ferventcoder deleted the maint/master/convert-crlf branch October 25, 2016 21:17
@Iristyle
Copy link
Contributor

Yeah, I'm not sure this will have an effectual change at all... unless you were also to add .gitattributes containing:

install_puppet.bat  text eol=crlf

@ferventcoder
Copy link
Contributor Author

ferventcoder commented Oct 29, 2016

Well as far as the commit having changes, you can see it did based that there are changes. However it may show differently to you when you pull locally if you don't have commit as is. That is a local configuration though, the PR showed the differences. :D

@ferventcoder
Copy link
Contributor Author

Unless I'm missing some context @Iristyle :)

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