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

Fix file.append #34432

Merged
merged 10 commits into from
Jul 5, 2016
Merged

Fix file.append #34432

merged 10 commits into from
Jul 5, 2016

Conversation

twangboy
Copy link
Contributor

@twangboy twangboy commented Jul 1, 2016

What does this PR do?

Fixes problem with extra newline characters in the file and the state return.

What issues does this PR fix or reference?

https://github.com/saltstack/zh/issues/783

Previous Behavior

Previously the salt state was iterating through each line in the source file and calling file.append. File.append checks for a newline character at the end of the file and adds one if it's missing. So there was a newline character being added on every line.

New Behavior

The salt state now sends a list of lines to be added to the file.append function.

Tests written?

Tested on Windows and Ubuntu
No

@cachedout
Copy link
Contributor

This concerns me.

The first thing that's happening here is that a file.append state that currently returns with no changes in 2015.8 will rewrite the entire file. I am not inclined to introduce a change like that into a point release.

I'm a little confused by your description as well. What I was seeing when testing this was that in the current incarnation of 2015.8, only an additional line was being added which is what I would expect. However, your PR seems to modify every single line in the file. Example:

              diff:
                  --- 

                  +++ 

                  @@ -1,2 +1,3 @@

                  -The quick brown fox

                  -jumped over

                  +The quick brown fox
                  +jumped over
                  +The fence

This is not at all the behavior that I would expect from file.append. For reference, here's the current behavior:

              diff:
                  --- 
                  +++ 
                  @@ -1,2 +1,3 @@
                   The quick brown fox
                   jumped over
                  +The fence

I am going to cc: @thatch45 and @terminalmage here for additional review.

@cachedout cachedout added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Jul 5, 2016
)
else:
ret['comment'] = 'File {0} is in correct state'.format(name)
ret['result'] = True
return ret

if len(append_lines):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the len() here?

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 len() determines if there were any lines appended to the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

right, but an empty list evaluates to False, and a list with stuff evaluates to True, making the len unnecessary:
https://docs.python.org/2/library/stdtypes.html#truth-value-testing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a dufus.....

@thatch45
Copy link
Contributor

thatch45 commented Jul 5, 2016

Looking at this, I wonder what we need the state changes for? I am just not following that.

@twangboy
Copy link
Contributor Author

twangboy commented Jul 5, 2016

@cachedout @thatch45 I think @jfindlay explained the problem pretty well in https://gist.github.com/jfindlay/45fb2de7b5970ef773033013632a36d6

I will do some more testing

@twangboy
Copy link
Contributor Author

twangboy commented Jul 5, 2016

@cachedout I believe your original file was missing a newline code after jumped over. file.append will add a newline code at the end of the file if it's missing.

@cachedout
Copy link
Contributor

@twangboy The point is that The quick brown fox should not have been modified.

@twangboy
Copy link
Contributor Author

twangboy commented Jul 5, 2016

@cachedout 'The quick brown fox' was not modified.... It's the diff comparison that thinks it was.... I submitted a fix. When it read the file before the append it was reading the line endings. After the append it was not. So, the compare showed that the line was changed.

@twangboy
Copy link
Contributor Author

twangboy commented Jul 5, 2016

Rebased

@thatch45 thatch45 merged commit 9e15337 into saltstack:2015.8 Jul 5, 2016
@twangboy twangboy deleted the fix_file.append branch August 30, 2016 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants