Ansible INI module #88

oleorhagen · 2024-01-10T14:03:01Z

A simple custom promise module wrapping the Ansible INI custom module, and simply passing on attributes from CFEngine into the Ansible module.

The one required attribute for the module is module_path which is the path to the Ansible INI module installed on the system.

Signed-off-by: Ole Petter <[email protected]>

promise-types/ini/ini.py

olehermanse · 2024-01-10T17:15:29Z

@oleorhagen The way I see it, there are roughly 4 approaches for how to install dependencies:

Dependencies are downloaded by cfbs and included as part of the deployed policy set.
The hub downloads dependencies and clients fetch them from the hub.
Each client downloads and installs needed dependencies for itself.
Leave it up to the users to install dependencies / ensure they have them already.

Or some combination of those. All approaches could be implemented and valuable, but I lean more towards 2, or some combination of 2 and 3. They allow the hub / client to keep things up to date if desirable, and to make some choices about what is needed (version of dependencies needed).

To get the ball rolling, why not start with including some small policy to install ansible and the ini module in this CFEngine Build module?

oleorhagen · 2024-01-11T09:23:33Z

To get the ball rolling, why not start with including some small policy to install ansible and the ini module in this CFEngine Build module?

Sounds like a good idea to me. KISS is the way for me here :p

oleorhagen · 2024-01-12T13:52:45Z

For now I've added a cfbs.json file in the directory. However, I see all other modules are incorportated into the top-level cfbs.json. I'm guessing you want me to follow that example?

olehermanse · 2024-01-12T13:55:15Z

For now I've added a cfbs.json file in the directory. However, I see all other modules are incorportated into the top-level cfbs.json. I'm guessing you want me to follow that example?

@oleorhagen Yea. Either that, or you can also develop the module in your own repo, if you prefer.

olehermanse · 2024-01-12T13:59:38Z

promise-types/ini/example.cf

@@ -0,0 +1,79 @@
+promise agent ini


@oleorhagen sorry that we don't have a good automatic formatter for policy language (yet). Please take a look at the style guide here and try to align with it (doesn't have to be perfect):

https://docs.cfengine.com/docs/master/examples-tutorials-writing-and-serving-policy-policy-style.html#top

Did my best. See latest push

promise-types/ini/README.md

craigcomstock

cool! thanks for this work!

promise-types/ini/example.cf

craigcomstock · 2024-01-16T16:34:59Z

promise-types/ini/example.cf

+
+  ini:
+    "/tmp/ini/test.toml"
+      module_path => "/tmp/cfe/ini_file.py",


honestly I wouldn't want to always have to specify this path.

I would suggest moving the logic to retrieve ini_file.py into the python even though I don't like that for "visibility" sake.

Yeah, I don't think we should have to specify module_path on the individual promise. That should be handled by setting up the promise type.

promise-types/ini/ini.py

promise-types/ini/README.md

promise-types/ini/enable.cf

promise-types/ini/example.cf

nickanderson · 2024-01-16T18:02:40Z

promise-types/ini/example.cf

+
+  ini:
+    "/tmp/ini/test.toml"
+      module_path => "/tmp/cfe/ini_file.py",


Yeah, I don't think we should have to specify module_path on the individual promise. That should be handled by setting up the promise type.

promise-types/ini/ini.py

oleorhagen · 2024-02-20T08:51:13Z

@olehermanse feel free to have a last lookover

craigcomstock

I think we must make the module_path default more dynamic based on platform/host information.

craigcomstock · 2024-02-20T16:17:36Z

libraries/python/cfengine.py

@@ -384,6 +384,9 @@ def _handle_evaluate(self, promiser, attributes, request):
        except Exception as e:
            self.log_critical(
                "{error_type}: {error}".format(error_type=type(e).__name__, error=e)
+                + e.message


seems like this would read easier and fit in one line.

then again, what does black say? yeah, black seems to agree with me. Maybe run all your python files through black

Hmm, this is black. But I'm guessing it's a version thing then. I used 19. but don't know what your standard is

oh dang. the opinionated tool changes its opinions... :( I have 23.3.0 from brew on macos. Anyhow, I do like it better on one line when it fits since it is a single expression, essentially a ternary expression.

promise-types/ini/README.md

promise-types/ini/example.cf

promise-types/ini/ini.py

craigcomstock · 2024-02-20T16:28:05Z

promise-types/ini/ini.py

+        if not meta.get("promise_type"):
+            raise ValidationError("Promise type not specified")
+
+        # TODO - does not work


might be nice to remove this or state why it doesn't work so we know what to look for when fixing.

is it important that it does not work? so we are not validating that module_path is specified?

craigcomstock · 2024-02-20T16:36:22Z

promise-types/ini/ini.py

+            % (promiser, attributes, meta)
+        )
+
+        if "module_path" not in attributes:


Related to the TODO above where validation "does not work" here we provide a reasonable default? Doesn't seem like it actually since we are assuming here python3.11 which is a wild assumption for many different platforms.

Can we be smarter here about the path?

I had to pip3 install ansible in order to get ini_file.py. pip3 install ansible-core didn't do enough.

If I do pip3 show ansible I get an idea of where to look that is dependent on the install which might be better and something we can do programmatically via python?

? pip3 show ansible Name: ansible Version: 9.2.0 Summary: Radically simple IT automation Home-page: https://ansible.com/ Author: Ansible, Inc. Author-email: [email protected] License: GPL-3.0-or-later Location: /usr/local/lib/python3.11/site-packages Requires: ansible-core Required-by:

I get your point, and I agree. However, I won't have time to finish this, and it seems the other modules too write in their Readme's that the prerequisites need to be installed.

I fiddled around with how to provide this, and tried a few different things:

Having the path in the promise agent ini setup, but this apparently is not valid syntax

Having it in the meta section in the promise - but this does not get passed to the custom promise module (only

Hardcoding it as I've done now.

Dynamically would have been the sweetest though

I had to pip3 install ansible in order to get ini_file.py. pip3 install ansible-core didn't do enough.

This is true, if we do not provide the file ourselves (Like in the original implementation where it was downloaded).

we could consider adding the file to the repo, and have it installed just as we do the custom promise module ini.py itself?

Right on. Two things then:

we need to update the requirements to include ansible since that's easier than having folks have to figure out where to get it.

make the path dynamic with some python code if possible, doesn't have to be done by you :)

This adds a custom promise module for wrapping the existing Ansible INI module. The module simply passes on values from the custom 'ini' promise type, to the Ansible module. With the example follows an example policy for downloading the required Ansible INI module. Signed-off-by: Ole Petter <[email protected]>

olehermanse · 2024-02-26T12:05:53Z

promise-types/ini/example.cf

+  meta:
+    "bundle_version" string => "0.0.1";
+    "promise_type"   string => "ini";


Using the meta promise type won't do anything for the promise module.

You could use the meta attribute per promise:

ini: "hello.ini" meta => { "report", "some=value" };

But it seems to me that the information you want to pass is more static, so would fit better if we make some room for it in the promise agent block.

Edit: This is currently not possible, the meta attribute does not get passed to the module. See custom promise type docs

olehermanse · 2024-02-26T12:07:05Z

promise-types/ini/example.cf

+  path        => "$(sys.workdir)/modules/promises/ini.py";
+  interpreter => "/usr/bin/python3";


We could enable setting environment variables for the module, or add another field of "data" to pass to the module here.

Yes, I like the data field idea

Add the exception message to the log output if present

9718a0b

Signed-off-by: Ole Petter <[email protected]>

olehermanse reviewed Jan 10, 2024

View reviewed changes

promise-types/ini/ini.py Outdated Show resolved Hide resolved

oleorhagen force-pushed the master branch 2 times, most recently from f287922 to a5c0131 Compare January 12, 2024 13:51

oleorhagen requested a review from olehermanse January 12, 2024 13:52

olehermanse reviewed Jan 12, 2024

View reviewed changes

oleorhagen force-pushed the master branch from a5c0131 to 76484ef Compare January 15, 2024 10:00

oleorhagen changed the title ~~RFC: Ansible INI module~~ Ansible INI module Jan 15, 2024

oleorhagen force-pushed the master branch from 76484ef to 503f29d Compare January 15, 2024 10:04

oleorhagen commented Jan 15, 2024

View reviewed changes

promise-types/ini/README.md Outdated Show resolved Hide resolved

oleorhagen force-pushed the master branch from 503f29d to 5ff4cff Compare January 15, 2024 10:19

oleorhagen requested a review from olehermanse January 15, 2024 11:34

oleorhagen force-pushed the master branch 3 times, most recently from 0c2447e to 565a484 Compare January 16, 2024 15:06

craigcomstock reviewed Jan 16, 2024

View reviewed changes

nickanderson requested changes Jan 16, 2024

View reviewed changes

nickanderson reviewed Jan 16, 2024

View reviewed changes

promise-types/ini/ini.py Show resolved Hide resolved

olehermanse reviewed Jan 18, 2024

View reviewed changes

promise-types/ini/ini.py Outdated Show resolved Hide resolved

oleorhagen force-pushed the master branch from 6106313 to 98c9959 Compare February 20, 2024 08:51

oleorhagen requested a review from olehermanse February 20, 2024 08:51

olehermanse requested review from craigcomstock and nickanderson February 20, 2024 14:06

craigcomstock requested changes Feb 20, 2024

View reviewed changes

oleorhagen force-pushed the master branch from 2ac4e0b to c43285f Compare February 21, 2024 11:39

olehermanse reviewed Feb 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ansible INI module #88

Ansible INI module #88

oleorhagen commented Jan 10, 2024 •

edited

Loading

olehermanse commented Jan 10, 2024

oleorhagen commented Jan 11, 2024

oleorhagen commented Jan 12, 2024

olehermanse commented Jan 12, 2024

olehermanse Jan 12, 2024

oleorhagen Jan 15, 2024

craigcomstock left a comment

craigcomstock Jan 16, 2024

nickanderson Jan 16, 2024

nickanderson Jan 16, 2024

oleorhagen commented Feb 20, 2024

craigcomstock left a comment

craigcomstock Feb 20, 2024

craigcomstock Feb 20, 2024

oleorhagen Feb 21, 2024

craigcomstock Feb 21, 2024

craigcomstock Feb 20, 2024

craigcomstock Feb 20, 2024

oleorhagen Feb 21, 2024

oleorhagen Feb 21, 2024

craigcomstock Feb 21, 2024

olehermanse Feb 26, 2024 •

edited

Loading

olehermanse Feb 26, 2024

oleorhagen Feb 26, 2024

		path => "$(sys.workdir)/modules/promises/ini.py";
		interpreter => "/usr/bin/python3";

Ansible INI module #88

Are you sure you want to change the base?

Ansible INI module #88

Conversation

oleorhagen commented Jan 10, 2024 • edited Loading

olehermanse commented Jan 10, 2024

oleorhagen commented Jan 11, 2024

oleorhagen commented Jan 12, 2024

olehermanse commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craigcomstock left a 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

oleorhagen commented Feb 20, 2024

craigcomstock left a 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

olehermanse Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleorhagen commented Jan 10, 2024 •

edited

Loading

olehermanse Feb 26, 2024 •

edited

Loading