Adding etc_hosts module #19283

jtyr · 2016-12-13T10:45:37Z

ISSUE TYPE

New Module Pull Request.

COMPONENT NAME

hosts

ANSIBLE VERSION

For Ansible 2.3.

SUMMARY

This module adds the ability to add, remove and modify records in the /etc/hosts file.

jtyr · 2016-12-13T10:45:37Z

Migrated from ansible/ansible-modules-extras#3457

jtyr · 2016-12-13T11:22:36Z

@drybjed I have moved this PR from the obsolete ansible-modules-extras project to here and would like to ask you to review it. You can still use this playbook for the tests.

abadger · 2017-01-19T17:40:16Z

lib/ansible/modules/system/hosts.py

+
+DOCUMENTATION = '''
+---
+module: hosts


Name seems a little too generic. Maybe etc_hosts?

abadger · 2017-01-19T17:41:12Z

lib/ansible/modules/system/hosts.py

+        argument_spec=dict(
+            ip=dict(required=True),
+            hostname=dict(),
+            alias=dict(type='raw'),


list might be a better type and then just leave off the space separated feature (comma separated should work IIRC)

sivel · 2017-01-19T17:42:43Z

lib/ansible/modules/system/hosts.py

+from ansible.module_utils._text import to_bytes
+import os
+import re
+import tempfile


These imports are incorrectly placed above the DOCUMENTATION/EXAMPLES/etc strings. The ansible standard is that these imports go below the documentation, and not at the top of the file.

I don't as of yet see the requested change. Did you push it?

About to push it ;o)

Just pushed it.

abadger · 2017-01-19T17:44:06Z

lib/ansible/modules/system/hosts.py

+from ansible.module_utils._text import to_bytes
+import os
+import re
+import tempfile


imports should go below the "ansible data variables" (DOCUMENTATION, RETURNS, etc). That keeps code that is used in the module together instead of separating it with many lines that aren't used in the module at all. Imports are usually broken into three parts, first stdlib, then 3rd party (none here), and finally from the project itself.

Other note, need to add metadata.

I think you mentioned yesterday that PEP8 is allowing variables above imports. The reality is different:

E402 module level import not at top of file

That makes me very sad to produce a module which is not PEP8 compliant ;o(((((((((((((((((

Ehm, it's PEP8 compliant now ;o)

abadger · 2017-01-19T17:45:35Z

One thing that always comes up with modules like this are "Why is it better to have a module than to use template (or even lineinfile)?" So be prepared for that question :-)

jasperla · 2017-01-20T07:21:05Z

lib/ansible/modules/system/etc_hosts.py

+        YAML list or a string containing comma separated list.
+  path:
+    required: false
+    default: /etc/hosts


Wouldn't it be better to adjust the name of the module to hosts ? It seems odd to effectively hardcode the filepath in the module name, while at the same time allowing to adjust the file path. This module might also be extended to add support for managing the Windows hosts file,

Furthermore both Puppet and Salt call their analogue type/modules also hosts.

Edit: I see this module used to be named hosts and was renamed after @abadger his feedback. However I wonder if Windows support and the more common naming in the larger ecosystem was considered at the time.

I have changed the name of the module from hosts to etc_hosts as suggested by @abadger because the name hosts seems to be too generic. I'm open to suggestions if you can think of a better name than hosts or etc_hosts.

I think hosts is a name that does not violate the principle of least surprise. I merely wanted to point the rest of the cfg mgmt ecosystem and possible Windows support. Let's see what @abadger thinks about it given the above.

I like 'etc_hosts' > 'hosts'. Especially if the intention is to support just unix style /etc/hosts.

If other formats/types is in the future, maybe 'hosts_file' ?

@jtyr is it the intention to only support non-windows?

alikins · 2017-01-26T15:10:41Z

lib/ansible/modules/system/etc_hosts.py

+    # Change file attributes if needed
+    if os.path.isfile(module.params['path']):
+        file_args = module.load_file_common_arguments(module.params)
+        changed = module.set_fs_attributes_if_different(file_args, changed)


Will this handle a 'chattr +i /etc/hosts' or otherwise immutable /etc/hosts (docker does some variant of that I think?)

Yes, it will allow to specify the attributes option as in the file module.

alikins · 2017-01-26T15:13:24Z

lib/ansible/modules/system/etc_hosts.py

+        changed = module.set_fs_attributes_if_different(file_args, changed)
+
+    # Print status of the change
+    module.exit_json(changed=changed, state=state, diff=diff)


I'd like to see the return include a data structure of /etc/hosts content. Maybe a serializeable version of the EtcHosts object? (not a requirement, just a nice to have)

For what purpose?

register it in a playbook and use it later. The module has already parsed the file into a object (self.lines and its records). That avoids the need to refetch and do string parsing to inspect the results.

I don't think so that full hosts file should be returned by the module. I would say that only the added, removed, modified result should be returned.

that is why we have a 'diff' facility for modules

alikins · 2017-01-26T15:19:35Z

Code/docs looks good to me. Have a couple of minor suggestions, but LGTM.

alikins · 2017-01-26T15:21:54Z

lib/ansible/modules/system/etc_hosts.py

+                    line['valid'] and
+                    line['ip'] == self.params['ip'] and (
+                        self.params['hostname'] is None or
+                        line['hostname'] == self.params['hostname'])):


I would be tempted to extract this check into a method or two. (To simplify reading the logic, and also to make it easier to easier to unit test).

abadger · 2017-02-02T15:10:01Z

lib/ansible/modules/system/etc_hosts.py

+'''
+
+
+class EtcHosts:


SHould inherit from object because of python2

Isn't that the default?

@abadger I can fix that if that's really so important. But there is another 39 modules using exactly the same syntax and they don't seem to break anything:

$ egrep -R '^\s*class .[^(]*:' * cloud/docker/_docker.py:class ContainerSet: cloud/centurylink/clc_modify_server.py:class ClcModifyServer: cloud/centurylink/clc_blueprint_package.py:class ClcBlueprintPackage: cloud/centurylink/clc_firewall_policy.py:class ClcFirewallPolicy: cloud/centurylink/clc_alert_policy.py:class ClcAlertPolicy: cloud/centurylink/clc_aa_policy.py:class ClcAntiAffinityPolicy: cloud/centurylink/clc_server.py:class ClcServer: cloud/centurylink/clc_loadbalancer.py:class ClcLoadBalancer: cloud/centurylink/clc_server_snapshot.py:class ClcSnapshot: cloud/amazon/ec2_elb.py:class ElbManager: cloud/amazon/ecs_ecr.py:class EcsEcr: cloud/amazon/ecs_cluster.py:class EcsClusterManager: cloud/amazon/rds.py:class RDSConnection: cloud/amazon/rds.py:class RDS2Connection: cloud/amazon/rds.py:class RDSDBInstance: cloud/amazon/rds.py:class RDS2DBInstance: cloud/amazon/rds.py:class RDSSnapshot: cloud/amazon/rds.py:class RDS2Snapshot: cloud/amazon/cloudtrail.py:class CloudTrailManager: cloud/amazon/cloudfront_facts.py:class CloudFrontServiceManager: cloud/amazon/cloudformation_facts.py:class CloudFormationServiceManager: cloud/amazon/ecs_service_facts.py:class EcsServiceManager: cloud/amazon/lambda_event.py:class AWSConnection: cloud/amazon/ecs_taskdefinition.py:class EcsTaskManager: cloud/amazon/lambda_alias.py:class AWSConnection: cloud/amazon/ecs_task.py:class EcsExecManager: cloud/amazon/ec2_customer_gateway.py:class Ec2CustomerGatewayManager: cloud/amazon/ecs_service.py:class EcsServiceManager: cloud/google/gce.py:class LazyDiskImage: cloud/misc/xenserver_facts.py:class XenServerFacts: clustering/consul_acl.py:class Rules: clustering/consul_acl.py:class Rule: monitoring/icinga2_feature.py:class Icinga2FeatureHelper: network/dnsmadeeasy.py:class DME2: network/omapi_host.py:class OmapiHostManager: packaging/os/rpm_key.py:class RpmKey: packaging/language/maven_artifact.py:class MavenDownloader: remote_management/stacki/stacki_host.py:class StackiHost: web_infrastructure/jenkins_job.py:class JenkinsJob:

I would recommend updating to inherit from object. Just because others did it or are doing it, doesn't necessarily make it a thing we wish to have.

We are trying to improve and standardize on implementations, and the preference here is that we standardize on inheriting from object.

A future task can be to improve modules where this was not followed.

No, it doesn't become the default until Python3.0

abadger · 2017-02-02T16:30:50Z

Discussed at the meeting today. The module is as good as it can be but we're unable to come to an agreement between the author and the project on the remaining point of where the imports belong. Since the module's actions can be performed via other modules, we're choosing to decline this submission. It can be pushed to galaxy in a role if you wish, though, and others can install and use it from there.

abadger · 2017-02-21T20:14:32Z

At the Feb 9th meeting this cam up for a vote. Unfortunately, the vote came out against merging: -1:5, 0:3, +1:1. The rationale was due to redundancy with template and concerns about cross-platform maintainability. can still be hosted on galaxy in a role. We're discussing a proposal for an "ansible-installer" program which can make things hosted on galaxy more discoverable but that's something for the future.

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Dec 20, 2016

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 2, 2017

jimi-c removed the plugin label Jan 4, 2017

abadger mentioned this pull request Jan 19, 2017

Standing Agenda, Public Core Meeting Meeting - January 2017 ansible/community#148

Closed

abadger reviewed Jan 19, 2017

View reviewed changes

sivel reviewed Jan 19, 2017

View reviewed changes

abadger reviewed Jan 19, 2017

View reviewed changes

jtyr force-pushed the ansible-modules-extras/pull/3457 branch from f07ba1c to 8222d68 Compare January 20, 2017 00:00

jasperla reviewed Jan 20, 2017

View reviewed changes

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jan 25, 2017

alikins reviewed Jan 26, 2017

View reviewed changes

jtyr added 8 commits January 27, 2017 15:18

Adding hosts module

ac6d0dc

Adding unsafe_writes option for the atomic_move

930fe95

Making the module compatible with Python 3

9051bf6

Fixing typo in the error message

5318503

Changes suggested by abadger and sivel

393b554

Removing unused import

db37de7

Adding METADATA block

7c542e9

Moving imports to the top to be PEP8 compliant

53b7b3f

jtyr force-pushed the ansible-modules-extras/pull/3457 branch from 184a23c to 949acc5 Compare January 27, 2017 15:42

Adding IP address validation

07324ee

jtyr force-pushed the ansible-modules-extras/pull/3457 branch 2 times, most recently from cfb02fd to 07324ee Compare January 27, 2017 15:48

Allow to delete records by hostname

8a0517b

abadger reviewed Feb 2, 2017

View reviewed changes

abadger closed this Feb 2, 2017

Move imports to the 'right' place and using new class style

26c3e9f

abadger reopened this Feb 2, 2017

abadger mentioned this pull request Feb 2, 2017

Public Core Meeting Agenda - Feb 2017 ansible/community#150

Closed

abadger closed this Feb 21, 2017

jtyr mentioned this pull request Sep 21, 2017

env_file module #30685

Closed

ansible locked and limited conversation to collaborators Apr 26, 2019

Adding etc_hosts module #19283

Adding etc_hosts module #19283

Conversation

jtyr commented Dec 13, 2016

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

SUMMARY

jtyr commented Dec 13, 2016

jtyr commented Dec 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abadger commented Jan 19, 2017

jasperla Jan 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alikins Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alikins commented Jan 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtyr Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abadger commented Feb 2, 2017

abadger commented Feb 21, 2017

jasperla Jan 20, 2017 •

edited

Loading

alikins Jan 26, 2017 •

edited

Loading

jtyr Feb 2, 2017 •

edited

Loading