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

Add win_blockinfile module #52586

Open
wants to merge 13 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@marwatk
Copy link
Contributor

marwatk commented Feb 19, 2019

SUMMARY

This adds a win_blockinfile module for Windows similar to blockinfile for Unix.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

win_blockinfile

ADDITIONAL INFORMATION

I used win_lineinfile as a structural basis (I'm far from PowerShell proficient) and blockinfile as a base for the algorithm. It supports diff and check_mode.

It uses the old module style as indicated by the docs. If the new one is preferred I'm happy to refactor.

It has a complete set of integration tests and should functionally match blockinfile outside of the Windows specific changes like variable line termination support.

There are some almost duplicated functions from win_lineinfile that I'd prefer to share, but that would require editing two modules and a library at once. Additionally this module preserves a file's ACL whereas win_lineinfile does not.

@ansibot

This comment was marked as resolved.

Copy link
Contributor

ansibot commented Feb 19, 2019

The test ansible-test sanity --test line-endings [explain] failed with 10 errors:

test/integration/targets/win_blockinfile/files/default_block_only_crlf.txt:0:0: use "\n" for line endings instead of "\r\n"
test/integration/targets/win_blockinfile/files/file_with_lines.txt:0:0: use "\n" for line endings instead of "\r\n"
test/integration/targets/win_blockinfile/files/file_with_lines_after.txt:0:0: use "\n" for line endings instead of "\r\n"
test/integration/targets/win_blockinfile/files/file_with_lines_before.txt:0:0: use "\n" for line endings instead of "\r\n"
test/integration/targets/win_blockinfile/files/file_with_lines_bof.txt:0:0: use "\n" for line endings instead of "\r\n"
test/integration/targets/win_blockinfile/files/file_with_lines_custom_marker.txt:0:0: use "\n" for line endings instead of "\r\n"
test/integration/targets/win_blockinfile/files/file_with_lines_eof.txt:0:0: use "\n" for line endings instead of "\r\n"
test/integration/targets/win_blockinfile/files/file_with_lines_inverted_marker.txt:0:0: use "\n" for line endings instead of "\r\n"
test/integration/targets/win_blockinfile/files/no_trailing_newline.txt:0:0: use "\n" for line endings instead of "\r\n"
test/integration/targets/win_blockinfile/files/no_trailing_newline_bof.txt:0:0: use "\n" for line endings instead of "\r\n"

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/windows/win_blockinfile.py:108:161: E501 line too long (166 > 160 characters)

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/windows/win_blockinfile.py:0:0: E312 No RETURN provided

click here for bot help

@ansibot

This comment has been minimized.

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 19, 2019

@ansibot ansibot added support:core test and removed ci_verified labels Feb 19, 2019

@ansibot

This comment was marked as resolved.

Copy link
Contributor

ansibot commented Feb 19, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/modules/windows/win_blockinfile.py:154:0: mixed-line-endings Mixed line endings LF and CRLF

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/modules/windows/win_blockinfile.py:108:120: W291 trailing whitespace
lib/ansible/modules/windows/win_blockinfile.py:154:4: W292 no newline at end of file

click here for bot help

@ansibot ansibot added the ci_verified label Feb 19, 2019

@samdoran samdoran removed the needs_triage label Feb 19, 2019

@ansibot ansibot removed the ci_verified label Feb 19, 2019

@marwatk

This comment has been minimized.

Copy link
Contributor Author

marwatk commented Feb 19, 2019

ready_for_review

@dagwieers
Copy link
Member

dagwieers left a comment

Good to see this contribution.
Including integration tests and perfect documentation, no less !
Still a few remarks.

Show resolved Hide resolved lib/ansible/modules/windows/win_blockinfile.py Outdated
Show resolved Hide resolved lib/ansible/modules/windows/win_blockinfile.py Outdated
Show resolved Hide resolved lib/ansible/modules/windows/win_blockinfile.py Outdated
Show resolved Hide resolved lib/ansible/modules/windows/win_blockinfile.py Outdated
Show resolved Hide resolved lib/ansible/modules/windows/win_blockinfile.py
Show resolved Hide resolved lib/ansible/modules/windows/win_blockinfile.ps1 Outdated
Show resolved Hide resolved lib/ansible/modules/windows/win_blockinfile.ps1 Outdated
Show resolved Hide resolved lib/ansible/modules/windows/win_blockinfile.ps1 Outdated
Show resolved Hide resolved lib/ansible/modules/windows/win_blockinfile.ps1 Outdated

@ansibot ansibot added needs_revision and removed core_review labels Feb 20, 2019

@ansibot

This comment was marked as resolved.

Copy link
Contributor

ansibot commented Feb 20, 2019

The test ansible-test sanity --test line-endings [explain] failed with 2 errors:

test/integration/targets/win_blockinfile/files/utf_16_after.txt:0:0: use "\n" for line endings instead of "\r\n"
test/integration/targets/win_blockinfile/files/utf_16_orig.txt:0:0: use "\n" for line endings instead of "\r\n"

The test ansible-test sanity --test no-smart-quotes [explain] failed with 2 errors:

test/integration/targets/win_blockinfile/files/utf_16_after.txt:1:1: UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 0: invalid start byte
test/integration/targets/win_blockinfile/files/utf_16_orig.txt:1:1: UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 0: invalid start byte

The test ansible-test sanity --test shebang [explain] failed with 2 errors:

test/integration/targets/win_blockinfile/files/utf_16_after.txt:0:0: file starts with a UTF-16 (LE) byte order mark
test/integration/targets/win_blockinfile/files/utf_16_orig.txt:0:0: file starts with a UTF-16 (LE) byte order mark

click here for bot help

@marwatk

This comment has been minimized.

Copy link
Contributor Author

marwatk commented Feb 20, 2019

@dagwieers
Ok, other than those two BackupFile related comments it's been refactored and cleaned up to be more sane. Take another look and let me know.

@lucastheisen
Copy link

lucastheisen left a comment

A few idiomatic suggestions, but overall looks good.

Show resolved Hide resolved lib/ansible/modules/windows/win_blockinfile.ps1 Outdated
Show resolved Hide resolved lib/ansible/modules/windows/win_blockinfile.ps1 Outdated
Show resolved Hide resolved lib/ansible/modules/windows/win_blockinfile.ps1 Outdated
Show resolved Hide resolved lib/ansible/modules/windows/win_blockinfile.ps1 Outdated
@marwatk

This comment has been minimized.

Copy link
Contributor Author

marwatk commented Feb 21, 2019

@dagwieers License text updated.

@ansibot

This comment was marked as resolved.

Copy link
Contributor

ansibot commented Feb 25, 2019

@marwatk this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibot

This comment was marked as resolved.

Copy link
Contributor

ansibot commented Feb 25, 2019

@marwatk This PR was evaluated as a potentially problematic PR for the following reasons:

  • More than 50 changed files.
  • More than 50 commits.

Such PR can only be merged by human. Contact a Core team member to review this PR on IRC: #ansible-devel on irc.freenode.net

click here for bot help

@marwatk marwatk force-pushed the marwatk:win_blockinfile branch from b661c19 to 4102047 Feb 25, 2019

@marwatk

This comment has been minimized.

Copy link
Contributor Author

marwatk commented Feb 25, 2019

Ok, after a slight hiccup with rebasing this is updated to use the shared Backup-File implementation, which I think was the last issue.

ready_for_review

@dagwieers

This comment has been minimized.

Copy link
Member

dagwieers commented Feb 26, 2019

@marwatk I ported your ACL-functionality to Backup-File in #52987 as well.

@dagwieers
Copy link
Member

dagwieers left a comment

LGTM, includes tests and proper documentation.

@dagwieers

This comment has been minimized.

Copy link
Member

dagwieers commented Feb 26, 2019

One remark, the template and win_template modules use parameter newline_sequence for modifying the newline, while win_lineinfile and this module use parameter newline. I think we need to standardize this for all modules. The question is, do we use newline or newline_sequence.

The content is also different:

  • newline_sequence uses [ '\n', '\r', '\r\n']
  • newline uses [ 'unix', 'windows' ]

So whatever we decide to do, it will consist if a deprecation window.

Update: I have put this on the agenda for today's meeting.

@dagwieers

This comment has been minimized.

Copy link
Member

dagwieers commented Feb 27, 2019

So the question related to newline/newline_sequence is not a showstopper to get this merged IMO.
Can others please review this module ?

@marwatk

This comment has been minimized.

Copy link
Contributor Author

marwatk commented Mar 4, 2019

Any chance this will make 2.8?

@ansibot ansibot added the stale_ci label Mar 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.