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

For Unix-style line endings for scripts, add .env to .gitignore #29

Merged
merged 2 commits into from
Dec 10, 2020
Merged

For Unix-style line endings for scripts, add .env to .gitignore #29

merged 2 commits into from
Dec 10, 2020

Conversation

qmfrederik
Copy link
Contributor

Description

This PR forces Unix-style checkout for .sh files, and adds the .env file to .gitignore.

Why is this needed

Cloning the sandbox repository on Windows and running vagrant up provisioner eventually fails like this:

[...]
    provisioner: + ./generate-envrc.sh eth1
    provisioner: /usr/bin/env: ‘bash\r’: No such file or directory
The SSH command responded with a non-zero exit status. Vagrant
assumes that this means the command failed. The output for this command
should be in the log above. Please read the output to determine what
went wrong.

This is because the .sh files are treated as text files by git, and the line endings are converted to crlf on Windows. Because the .sh files are mounted into the Linux guest VMs, they would end up as with the crlf line ending on Linux, causing vagrant up to fail.

How Has This Been Tested?

Tested locally.

Because the scripts create symlinks in the /vagrant folder, you need to make sure that your Windows user has permissions to create symbolic links

How are existing users impacted? What migration steps/scripts do we need?

Fixes a bug.

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

.gitignore Outdated
@@ -1,2 +1,3 @@
envrc
out
.env
Copy link
Contributor

Choose a reason for hiding this comment

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

missing line-ending! :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leave it to me to mess up a 3-line PR ;-)

@@ -0,0 +1,2 @@
# Use Unix line endings for scripts
*.sh text eol=lf
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect this should be handled with autocrlf on your machine right? I wonder what happens if we also set * text eol=lf? Would git do the right thing...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with autocrlf is that it is a machine-wide setting, and I do want crlf line endings in most of my repositories. Just not here, since these files end up getting mounted into the Vagrant VMs.

* text would force git to treat all files in this repo as text files, which I guess is fine if you're sure you'll never check in binaries in this repo :-).

Based on my trial, it's just the .sh files which need to lf line endings. For example, the Dockerfile files also end up with crlf line endings, but docker can handle that just fine on Linux.

I don't have a strong preference, so anything that lets me just run vagrant up on Windows is fine with me :D.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah i see. This seems fine to me then.

@mmlb
Copy link
Contributor

mmlb commented Dec 7, 2020

@qmfrederik please amend your commits to certify dco.

Signed-off-by: Frederik Carlier <frederik.carlier@quamotion.mobi>
Signed-off-by: Frederik Carlier <frederik.carlier@quamotion.mobi>
@mmlb mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Dec 7, 2020
@gianarb gianarb merged commit 3a5445e into tinkerbell:master Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants