Skip to content
This repository has been archived by the owner. It is now read-only.

New module: blockinfile #832

Merged
merged 1 commit into from Nov 11, 2015
Merged

New module: blockinfile #832

merged 1 commit into from Nov 11, 2015

Conversation

@yaegashi
Copy link
Contributor

@yaegashi yaegashi commented Aug 13, 2015

blockinfile module is to insert/update/remove a text block surrounded by marker lines in a file, in the similar manner as lineinfile module.

I've published this module as Ansible role for almost one year (Galaxy, GitHub) and got some feedback (reviews/issues/stars). I think it has already been accepted by certain number of users.

yaegashi added a commit to yaegashi/ansible-role-blockinfile that referenced this pull request Aug 14, 2015
- Synced with the pull request to ansible-module-extras.
  ansible/ansible-modules-extras#832
- Added test-block.
yaegashi added a commit to yaegashi/ansible-role-blockinfile that referenced this pull request Aug 14, 2015
Synced with the pull request to ansible-module-extras.
ansible/ansible-modules-extras#832

Added test-block.
@yaegashi yaegashi closed this Aug 14, 2015
@yaegashi yaegashi reopened this Aug 14, 2015
@robynbergeron
Copy link
Contributor

@robynbergeron robynbergeron commented Sep 25, 2015

Hi @yaegashi --

Thanks for submitting this new module to Ansible Extras! This module is now in community review, a process that is open to all Ansible users. In order for this module to be approved, it must gain the following votes:

“works_for_me”: If you have tested the module thoroughly, including testing of all of the module’s options, and if the module works for you, please add “works_for_me” in the comments.

“passes_guidelines”: If you have gone through the module guidelines and the module meets all of the requirements, please add “passes_guidelines” in the comments. Guidelines are available here: http://docs.ansible.com/developing_modules.html#module-checklist

“needs_revision”: If the module fails to work for you, or if it doesn’t meet guidelines, please add “needs_revision” in the comments with details about what needs to be fixed.

When a module has both “works_for_me” and “passes_guidelines” tags, we will promote the module for inclusion in Ansible Extras. At this point, you will be expected to maintain the module by fixing bugs and evaluating pull requests in a timely manner.

Thanks again for submitting your Ansible module!

@jtyr
Copy link
Contributor

@jtyr jtyr commented Sep 28, 2015

It would be really nice if the code would be PEP8 compliant:

$ pep8 ./blockinfile.py 
./blockinfile.py:95:80: E501 line too long (100 > 79 characters)
./blockinfile.py:122:1: E302 expected 2 blank lines, found 1
./blockinfile.py:122:25: E231 missing whitespace after ','
./blockinfile.py:122:34: E231 missing whitespace after ','
./blockinfile.py:125:24: E231 missing whitespace after ','
./blockinfile.py:138:56: E231 missing whitespace after ','
./blockinfile.py:142:1: E302 expected 2 blank lines, found 1
./blockinfile.py:154:1: E302 expected 2 blank lines, found 1
./blockinfile.py:181:80: E501 line too long (82 > 79 characters)
./blockinfile.py:215:36: E701 multiple statements on one line (colon)
./blockinfile.py:216:36: E701 multiple statements on one line (colon)
./blockinfile.py:222:41: E701 multiple statements on one line (colon)
./blockinfile.py:230:28: E261 at least two spaces before inline comment
./blockinfile.py:265:1: E402 module level import not at top of file
./blockinfile.py:266:1: E402 module level import not at top of file

I would also recommend to rebase your changes into one commit only and use git push -f for any future updates of this PR.

@bcoca
Copy link
Member

@bcoca bcoca commented Sep 28, 2015

actually we like the code to be readable and maintainable, that is not tied to pep8 compliance, i specially find the 79 column rule to be detrimental and I don't find whitespaces around , to influence readability one way or another.

@jtyr
Copy link
Contributor

@jtyr jtyr commented Sep 28, 2015

@bcoca I believe that formatting the code with one style helps the readability and facilitates future contributions to the code. I understand that the strict length of the line can be detrimental (e.g. introducing new vars in order to be able to fit the expression in 79 chars, ...), but the rest of the PEP8 rules are IMHO reasonable to request for any new piece of code.

- If specified, the block will be inserted after the last match of
specified regular expression. A special value is available; C(EOF) for
inserting the block at the end of the file. If specified regular
expresion has no matches, EOF will be used instead.

This comment has been minimized.

@jtyr

jtyr Sep 28, 2015
Contributor

EOF should be C(EOF)

description:
- 'This module will insert/update/remove a block of multi-line text
surrounded by customizable marker lines
(default: "# {BEGIN/END} ANSIBLE MANAGED BLOCK").'

This comment has been minimized.

@jtyr

jtyr Sep 28, 2015
Contributor

I think that the "default: ..." should not be part of the description.


EXAMPLES = r"""
- name: insert/update "Match User" configuation block in /etc/ssh/sshd_config
blockinfile: dest=/etc/ssh/sshd_config block="Match User ansible-agent\nPasswordAuthentication no"

This comment has been minimized.

@jtyr

jtyr Sep 28, 2015
Contributor

I would use the block YAML format for all examples.

@jtyr
Copy link
Contributor

@jtyr jtyr commented Sep 28, 2015

My comments to the functionality:

  • I would appreciate to be able to set the state parameter instead of removing the block content.
  • I would be really nice to be able to use the task name in the marker by default (something like # {marker} {task_name}) in order to be able to specify multiple blocks in the same file without the need to manually set the marker parameter (assuming the tasks have different names).
@dobber
Copy link

@dobber dobber commented Sep 29, 2015

works_for_me
I've tested all options except the selinux part. Everything works as documented.

@yaegashi
Copy link
Contributor Author

@yaegashi yaegashi commented Oct 1, 2015

@jtyr Thanks much for the valuable comments, I'll update as you suggested.

Using the task name as the marker is a good idea, but there would be concerns about breaking compatibility with already existing playbooks. I'll try to implement it, though it would take some time for testing.

@yaegashi
Copy link
Contributor Author

@yaegashi yaegashi commented Oct 1, 2015

@dobber Thanks for testing! FYI there's a testing script with some test cases in ansible-role-blockinfile. Please see CONTRIBUTING.md for details.

(I'm wondering if there's a way to include unit tests for modules like this in the upstream)

@conorsch
Copy link

@conorsch conorsch commented Oct 2, 2015

@yaegashi Thanks for submitting this for inclusion, I've been using it a while, so works_for_me. The edits @jtyr mentioned would go a long way in making the module more intuitive., so consider adding them.

@yaegashi yaegashi force-pushed the yaegashi:module-blockinfile branch from c5e2284 to be9cd93 Oct 3, 2015
@yaegashi
Copy link
Contributor Author

@yaegashi yaegashi commented Oct 3, 2015

be9cd93: Updated, rebased on current devel branch with squash.

The updates include all suggestions from @jtyr except for the feature {task_name} in the maker template. Unfortunately there seems no way to get the task name from a module. Only Ansible version and user arguments are passed to a module via ANSIBLE_VERSION and MODULE_COMPLEX_ARGS in module_utils/basic.py. It might be worth requesting this enhancement for modules to the project...

@yaegashi yaegashi force-pushed the yaegashi:module-blockinfile branch from be9cd93 to 8d53416 Oct 3, 2015
@yaegashi
Copy link
Contributor Author

@yaegashi yaegashi commented Oct 3, 2015

8d53416: Updated again, includes yaegashi/ansible-role-blockinfile@760ab9e.

@dobber @conorsch Released ansible-role-blockinfile v0.6. New option state is added. Could you please test it with your playbooks? Thanks for your cooperation!

@mavit
Copy link
Contributor

@mavit mavit commented Oct 5, 2015

I've reviewed this against the checklist at http://docs.ansible.com/developing_modules.html#module-checklist. It failed on only the following two items, so needs_revision.

  • Does module use check_mode? Could it be modified to use it? Document it

    The module does use check_mode, but doesn't mention this in the documentation.

  • Return: document the return structure of the module

choices: [ 'EOF', '*regex*' ]
insertbefore:
required: false
default: null

This comment has been minimized.

@jtyr

jtyr Oct 6, 2015
Contributor

default: null should be default: None as per the module checklist.

This comment has been minimized.

@yaegashi

yaegashi Oct 10, 2015
Author Contributor

No, I'm opposed to it because string 'None' is passed to the module if you actually write None as in the following playbook.

---
- hosts: all
  gather_facts: no
  tasks:
    - blockinfile:
        dest: '{{playbook_dir}}/none.txt'
        block: block
        insertbefore: None

I think the checklist mentions None as a value the module gets from AnsbileModule.params. YAML spec says null should be used to represent None in Python.

This comment has been minimized.

@jimi-c

jimi-c Oct 12, 2015
Member

This is a minor point, but as the documentation block is for generating text, we actually want something to show up there, so "None" is actually correct (as a string). The actual default value is set in the module spec, which will be None there (which it is: insertbefore=dict(default=None),).

This comment has been minimized.

@mavit

mavit Oct 12, 2015
Contributor

In light of this, lets go back to needs_revision.

This comment has been minimized.

@yaegashi

yaegashi Oct 12, 2015
Author Contributor

@jimi-c Thanks for the clarification, I'll fix it. Please update DOCUMENTATION.yml to conform to that. It would be nice if we would also have examples for EXAMPLES and RETURN.

@thuandt
Copy link

@thuandt thuandt commented Oct 9, 2015

👍
I'm waiting for this module will merge to Ansible Extras.

@xrow
Copy link

@xrow xrow commented Oct 9, 2015

Usefull feature

@yaegashi
Copy link
Contributor Author

@yaegashi yaegashi commented Oct 11, 2015

@mavit Thanks for the review. I've been aware that two are missing in the module documentation.

  • Check mode document: Actually, almost all modules in both core and extras are missing it and it's quite unclear how one should document it in the module. I'll just add a note saying this module supports it, just as in cloudstack's ModuleDocFragment.
  • Return value document: I don't think it's mandatory to document it when the module returns no special values other than common return values, where you can read: "Here we document the values common to all modules, each module can optionally document its own unique returns." And I think it would be better to modify the module doc template to handle all conditions of all modules in "Return Values" section rather than to mention it in each module document. I've made a PR for that: ansible/ansible#12705.
yaegashi added a commit to yaegashi/ansible-role-blockinfile that referenced this pull request Oct 11, 2015
@yaegashi yaegashi force-pushed the yaegashi:module-blockinfile branch from 8d53416 to ba89ab8 Oct 11, 2015
@yaegashi
Copy link
Contributor Author

@yaegashi yaegashi commented Oct 11, 2015

ready_for_review

@robynbergeron
Copy link
Contributor

@robynbergeron robynbergeron commented Oct 12, 2015

Hi @yaegashi -- thanks for your thoughtful comments and PRs made WRT the module guidelines. And thanks to @mavit and @jtyr for your conscientious reviews! :)

I'd like to get feedback from @mavit and @jtyr on whether or not this now meets the module guidelines with the additions @yaegashi made. Or, if they are at least good enough for now given the questions @yaegashi raised re: the guidelines themselves -- provided @yaegashi is willing to come back and make a PR if needed once those questions are answered or ansible/ansible#12705 is merged.

@dobber or @conorsch -- or anyone else :) -- if you could review this one more time for functionality and mark it as works_for_me, that would be great -- since we've had quite a few updates to this PR since then.

Thanks everyone for putting your work and eyeballs into this PR so far -- it's really great to see! :)

@yaegashi yaegashi force-pushed the yaegashi:module-blockinfile branch from ba89ab8 to 592e300 Oct 12, 2015
@yaegashi
Copy link
Contributor Author

@yaegashi yaegashi commented Oct 12, 2015

ready_for_review

yaegashi referenced this pull request in yaegashi/ansible-role-blockinfile Oct 12, 2015
@jtyr
Copy link
Contributor

@jtyr jtyr commented Oct 12, 2015

I think it's good enough now.

  • works_for_me
  • passes_guidelines
@robynbergeron
Copy link
Contributor

@robynbergeron robynbergeron commented Oct 13, 2015

This module has received all of the necessary votes to be included in Ansible Extras. Congratulations! Thanks to everyone for your reviews, and to @yaegashi for all your work here :)

A friendly reminder to @yaegashi - by submitting this module, you agree to maintain the module by fixing bugs and evaluating pull requests in a timely manner. If you are unable to do so, we may ask other community members to maintain the module, or we may deprecate the module altogether.

Thanks again for contributing this module to the Ansible community.

Marking for inclusion -- cc @ansible/core team :)

@yaegashi
Copy link
Contributor Author

@yaegashi yaegashi commented Oct 13, 2015

@robynbergeron Oh, thanks for your decision to include, and much thanks to people who participated in the review of this PR! Of course I'll look after my module from now on, make other PRs for improvements this PR couldn't cover.

It would be nice if we could carry out more strict review process more easily by introducing some automation on the infrastructure of GitHub issue tracker some time in future. At least it should be possible to add/remove triage labels on PRs based on keywords in comments.

@bcoca bcoca modified the milestone: next Oct 19, 2015
@gregdek
Copy link
Contributor

@gregdek gregdek commented Oct 27, 2015

@yaegashi that exact thing is what we're working on. :)

@m3nu
Copy link

@m3nu m3nu commented Nov 9, 2015

Also tested it today and worked as expected. Even with loops, items, etc.

@ghost
Copy link

@ghost ghost commented Nov 10, 2015

For me the functionality of this module should be part of ansible-modules-core and probably merged with template to do inline block replace in same file with regular expression for begin and end marks.
see ansible/ansible-modules-core#2441

def main():
module = AnsibleModule(
argument_spec=dict(
dest=dict(required=True, aliases=['name', 'destfile']),

This comment has been minimized.

@abadger

abadger Nov 11, 2015
Member

2.0 has a new type='path' that will expand user and expand vars for that parameter.

marker1 = re.sub(r'{mark}', 'END', marker)
if present and block:
# Escape seqeuences like '\n' need to be handled in Ansible 1.x
if ANSIBLE_VERSION.startswith('1.'):

This comment has been minimized.

@abadger

abadger Nov 11, 2015
Member

So this could be removed after a bit (since in the official repository, it won't be shipped with ansible-1.x anyway?)

@abadger
Copy link
Member

@abadger abadger commented Nov 11, 2015

I saw a few things that could be changed to enhance this module but neither are blockers. I'm going to merge now and you can submit those as changes to the module if you like in the future.

abadger added a commit that referenced this pull request Nov 11, 2015
@abadger abadger merged commit 1ddc321 into ansible:devel Nov 11, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ktaragorn
Copy link

@ktaragorn ktaragorn commented Jan 11, 2016

I came here all prepared to talk up this module only to find that it is already included :)

It helped me work around ansible/ansible#13763 and I am very grateful

@jl-montes
Copy link

@jl-montes jl-montes commented Feb 15, 2016

hi team, has anyone else gotten an error downloading the blockinfile from ansible-galaxy?
I've downloaded other items from ansible-galaxy, but this one generated a file download error

$ ansible-galaxy install yaegashi.blockinfile

@ktaragorn
Copy link

@ktaragorn ktaragorn commented Feb 16, 2016

@jl-montes It is already included as a part of the proverbial "batteries" in ansible v2. You do not need to install it seperately.

@jl-montes
Copy link

@jl-montes jl-montes commented Feb 16, 2016

I have v1.9.4 locally, have been ramping up with the tool for about 2 weeks. I was able to wget the tar file and untar in my local /roles folder, then declare the role in a playbook and call the function as a work-around.

Not ready to jump to v2.0 yet :-)

@danbohea
Copy link

@danbohea danbohea commented Feb 18, 2016

@jl-montes Yep, I'm experiencing the same issue trying to install blockinfile from Galaxy. For now I'm working round it by specifying the GitHub path in my roles file rather than using the shorter Galaxy syntax:

- src: https://github.com/yaegashi/ansible-role-blockinfile
  name: blockinfile
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet