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.

Sign up for GitHub

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

Jump to bottom

Convert CRLF to LF #1806

Merged
merged 4 commits into from
May 31, 2023
Merged

Convert CRLF to LF #1806

merged 4 commits into from
May 31, 2023

Conversation

NimJay
Copy link
Collaborator

@NimJay NimJay commented May 29, 2023 •
edited
Loading

Background

  • This fixes issue 1347 — which pull-request 1688 didn't fully fix.
  • LF (invisible character) is used by Mac/Linux to denote the end of a line in a file.
  • CRLF (invisible characters) is used by Windows to denote the end of a line in a file.
  • What the .gitattributes file from pull-request 1688 does:
    • it ensures newly committed text files (i.e., not binaries/PNGs/JPEG) use LF in remote — not necessarily locally
    • it does not convert existing files that use CRLF to LF
    • it also used CRLF locally on Windows and LF locally on Mac and Linux
  • I used git ls-files --eol to check which text files (ignoring png/jpeg/jar/tgz/etc.) were still using CRLF. I found that the following files were still using CRLF:
    • src/cartservice/cartservice.sln
    • src/adservice/gradlew.bat
  • @marcghanime mentioned in this comment that they were able to fix the issue by converting src\adservice\gradlew.bat (and 2 other files) to LF (from CRLF).

Change Summary

  • I ran: git add --renormalize . to convert the following files to LF (from CRLF):
    • src/cartservice/cartservice.sln
    • src/adservice/gradlew.bat
  • change the .gitattributes file such that even local files on Windows use LF — not CRLF.

Testing Procedure

  • We should ensure:
    • GitHub checks of this PR were able to successfully build the adservice Dockerfile
    • we are able to docker build the adservice on Windows

@NimJay NimJay requested a review from a team as a code owner May 29, 2023 15:09
@NimJay NimJay mentioned this pull request May 29, 2023
Closed
@github-actions
Copy link

🚲 PR staged at http://35.226.210.39

1 similar comment
@github-actions
Copy link

🚲 PR staged at http://35.226.210.39

bourgeoisor
bourgeoisor approved these changes May 29, 2023
View reviewed changes
Copy link
Member

@bourgeoisor bourgeoisor left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for checking this!

@NimJay
Enforce LF in .gitattributes
0926c9d 
Prior to this change, local files on Window were still using CRLF.
@github-actions
Copy link

🚲 PR staged at http://35.226.210.39

@github-actions
Copy link

🚲 PR staged at http://35.226.210.39

@NimJay
Copy link
Collaborator Author

NimJay commented May 31, 2023 •
edited
Loading

Thank you, @bourgeoisor, for the quick review.
I've included another minor change (in the .gitattributes file that ensures Windows uses LF locally).
I tested skaffold with the change on a Window machine. Worked.
This comment has more info.
Merging.

@NimJay NimJay merged commit 046c2ef into main May 31, 2023
@NimJay NimJay deleted the nimjay-lf branch May 31, 2023 14:33
sitaramkm pushed a commit to sitaramkm/microservices-demo that referenced this pull request Aug 24, 2023
@NimJay @sitaramkm
Convert CRLF to LF (GoogleCloudPlatform#1806)
946090f 
* Convert CRLF to LF

* Enforce LF in .gitattributes

Prior to this change, local files on Window were still using CRLF.
mrcrgl pushed a commit to fiberfjord/microservices-demo that referenced this pull request Sep 11, 2023
@NimJay @mrcrgl
Convert CRLF to LF (GoogleCloudPlatform#1806)
72414f9 
* Convert CRLF to LF

* Enforce LF in .gitattributes

Prior to this change, local files on Window were still using CRLF.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bourgeoisor bourgeoisor bourgeoisor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

./gradlew: not found local cluster deployment
2 participants
@NimJay @bourgeoisor