raymondbutcher / pretf

Generate Terraform code with Python
https://pretf.readthedocs.io/
MIT License
104 stars 14 forks source link

Proposal for defining Terraform blocks using Python objects #33

Open mdawar opened 5 years ago

mdawar commented 5 years ago

Hello,

I would like to ask you about adding new features to pretf before creating a pull request, these new features will let us define Terraform blocks using Python code without using the block function, for example let's take the example found in the documentation:

from pretf.api import block

def pretf_blocks(var):

    group = yield block("resource", "aws_iam_group", "admins", {
        "name": "admins",
    })

    for name in var.admin_user_names:

        user = yield block("resource", "aws_iam_user", name, {
            "name": name,
        })

        yield block("resource", "aws_iam_user_group_membership", name, {
            "user": user.name,
            "groups": [
                group.name,
            ],
        })

Compare using the block function to this code below:

from pretf.terraform import resource

def pretf_blocks(var):

    group = yield resource.aws_iam_group.admins(
        name = "admins"
    )

    for name in var.admin_user_names:

        user = yield resource.aws_iam_user[name](
            name = name
        )

        yield resource.aws_iam_user_group_membership[name](
            user = user.name,
            groups = [
                group.name
            ]
        )

Also users can import Terraform resources dynamically without them being explicitly defined:

# Handled by the resource module's __getattr__
# Requires Python3.7+
from pretf.terraform.resource import aws_instance, route53_record, linode_instance

# Same as
from pretf.terraform import resource

resource.aws_instance
resource.route53_record
resource.linode_instance

All of these objects can be instances of a class that defines __getattr__ to handle block labels and when called the passed arguments will be the block's body:

>>> from pretf.terraform import resource, data, output
>>> ubuntu_ami = data.aws_ami.ubuntu(most_recent=True)
>>> ubuntu_ami
block('data', 'aws_ami', 'ubuntu', {'most_recent': True})
>>> ec2_instance = resource.aws_instance.web(ami=ubuntu_ami.id, instance_type='t2.micro')
>>> ec2_instance
block('resource', 'aws_instance', 'web', {'ami': Interpolated('data.aws_ami.ubuntu.id'), 'instance_type': 't2.micro'})
>>> instance_number = '01'
>>> # For dynamic values we can use the square brakets for example
... output[f'instance_{instance_number}_ip'](value=ec2_instance.public_ip)
block('output', 'instance_01_ip', {'value': Interpolated('aws_instance.web.public_ip')})

As you can see these objects return a block, so there's no need to change anything in the core functionality and also this new syntax can be optional, people can just stick to using the block function.

I have tested this code on a new branch in my fork if you want to check it out, of course this is just an example and not the final code and without tests.

So what do you think?

raymondbutcher commented 5 years ago

Hi,

Firstly, thank you very much for thinking about this and trying to improve the project!

When I started this project and thought about various APIs, I discounted this style because reserved words in Python force multiple styles of usage. This is probably the main reason why the API ended up using dictionaries and strings.

This won't run because lambda is a reserved word:

from pretf.terraform import module

def pretf_blocks(var):
    yield module.role(...)
    yield module.lambda(...)

So you'd have to do:

from pretf.terraform import module

def pretf_blocks(var):
    yield module.role(...)
    yield module["lambda"](...)

It's a bit messy and confusing. You might decide to update the other module for consistency:

from pretf.terraform import module

def pretf_blocks(var):
    yield module["role"](...)
    yield module["lambda"](...)

But I still think that isn't very nice to read or write. The word lambda has basically infected the file and now it's a mix of styles, or a less desirable style.

It's also possible for a kwarg to be a reserved word. Here's the ugly solution for that:

from pretf.terraform import module

def pretf_blocks(var):
    yield module.something(**{
        "from": "thing",
    })

The documentation would also be more complicated by having these different ways of doing things, having to explain why there are multiple ways, and when to use each one.

So... I don't know. I like the attribute style, it is definitely cleaner when it works, but I don't like what happens when it doesn't work.

raymondbutcher commented 5 years ago

I'm leaning towards yes for including this. It is optional, as you said, and it's nice when you don't need to use any reserved words.

The reserved kwargs issue I mentioned could be improved slightly by accepting an optional dictionary as the first and only positional argument.

from pretf.block import module

def pretf_blocks(var):
    yield module.something({
        "from": "thing",
    })

Also thinking it should be the pretf.block or pretf.blocks module.

I still want to think about this some more, and see what you think about what I've written.

mdawar commented 5 years ago

@raymondbutcher Thank you for reviewing this proposal, well it's your call, it was just an idea to improve the syntax.

About the reserved keywords, I think that they won't be a problem unless the user is creating a resource with a similar name and that should be the user's problem because they should avoid these keywords, otherwise all of the resources names and data sources names of Terrafrom are prefixed with the provider's name so there won't be any conflicts, but that's not the case for keyword arguments as you pointed out but do you have any example of Terraform arguments that conflict with Python's reserved keywords?

>>> import keyword
>>> keyword.kwlist
['False', 'None', 'True', 'and', 'as', 'assert', 'async', 'await', 'break', 'class', 'continue', 'def', 'del', 'elif', 'else', 'except', 'finally', 'for', 'from', 'global', 'if', 'import', 'in', 'is', 'lambda', 'nonlocal', 'not', 'or', 'pass', 'raise', 'return', 'try', 'while', 'with', 'yield']

We can accept a dict as the first argument as you suggested or we can also check the kwargs for arguments starting with an underscore and check if we remove the underscore if the keyword is reserved key in keyword.kwlist and pass it without the underscore.

And about the module name yes any name you suggest is fine pretf.block or pretf.blocks.

I'll let you decide, if you're happy with the project as it is right now we can just close this issue, there's no problem at all with using the block function.

mcintyre321 commented 5 years ago

Could you make use of https://github.com/minamijoyo/tfschema to generate the type and type hints?

raymondbutcher commented 5 years ago

That looks interesting, but I don't think it would fit in well here.

If you're talking about having the types pre-built and packaged with Pretf, I think that adds too much of a build/maintenance burden. It would require a Terraform project using every single provider so that tfschema could inspect them. Also, custom providers would not be included.

If you're talking about users having tfschema installed and it somehow hooks into Pretf, I'm not sure how that would work but I'm pretty sure it would be annoying to set up.