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

Detect and preserve line endings #48380

Closed
wants to merge 3 commits into
base: 2017.7
from

Conversation

Projects
None yet
4 participants
@twangboy
Contributor

twangboy commented Jun 29, 2018

What does this PR do?

Detects the line endings for the file and uses those to parse the file. Preserves the line endings when the file is written.

What issues does this PR fix or reference?

#48110

Previous Behavior

On Windows, files with Unix line endings were writing back an empty file.

New Behavior

Now the file is edited properly.

Tests written?

No

Commits signed with GPG?

Yes

@salt-jenkins salt-jenkins requested a review from saltstack/team-windows Jun 29, 2018

@twangboy twangboy requested a review from terminalmage Jun 29, 2018

twangboy added some commits Jun 29, 2018

Add unit tests for the line function
Cover both windows and unix line endings

@twangboy twangboy requested a review from saltstack/team-windows Jul 3, 2018

@terminalmage

This was fixed very differently in develop a few weeks ago in #46291. I'm hesitant to merge something that will so significantly conflict with newer code.

Maybe we should instead work to backport #46291?

@twangboy

This comment has been minimized.

Contributor

twangboy commented Jul 6, 2018

But I wrote tests and everything...

@twangboy

This comment has been minimized.

Contributor

twangboy commented Jul 6, 2018

I'll test the one in develop...

@twangboy

This comment has been minimized.

Contributor

twangboy commented Jul 6, 2018

@terminalmage After some finagling, I was able to get the files on develop to run on 2017.7. It modified the files as expected.

@twangboy twangboy closed this Jul 6, 2018

@twangboy

This comment has been minimized.

Contributor

twangboy commented Jul 6, 2018

So, we need to backport #46291 to 2017.7

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jul 9, 2018

@twangboy when you say "after some finagling", does that mean changes were needed in order to apply #46291? And if so, what?

It might be best if you just backport the change to 2017.7 instead of me trying to retrace your steps.

We'd probably still like to have some tests around this area, so if your tests are applicable to the new code, then please add them to your back-port PR.

@twangboy

This comment has been minimized.

Contributor

twangboy commented Jul 10, 2018

@rallytime It was mostly all the salt.utils.* stuff where stuff was moved into individual salt.utils files. It was tracking all those down. I don't have the code anymore. If you want, I'm happy to do it again.

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jul 10, 2018

@twangboy Yes, if you could work through back-porting that change to 2017.7, that would probably be best. Thanks!

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jul 10, 2018

@twangboy Actually, nevermind, I was trying to back-port that to 2017.7 and it was a mess. 2018.3 was more doable and I have backported it there. Can you review it and test it and make sure it fixes #48110?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment