Add triggers to the null_resource

[knuckolls]

Looking through issues it appears that a fair amount of people, including people at hashicorp, use the null_resource as a container for running provisioners at specific times in the graph heirarchy. This is because provisioners are not a first class concept in the graph like resources are.

There's a problem with this, the null_resource will never re-run unless it's tainted. So for example if an aws_instance gets tainted or rolled for one reason or another and someone is using a null_resource to provision things on that box at a later stage of graph execution then a subsequent apply won't know to re-run the null_resource provisioners without also tainting it as well. This becomes a hassle in sufficiently large terraform codebases, imo.

What is introduced here is the concept of an optional, ForceNew, ListMap worth of "triggers" that write computed values to the statefile. When these computed values change, like the private_ip of an aws_instance, then the null_resource participates in the graph execution as would be expected and re-runs the provisioners.

After I wrote this I found that it's conceptually similar to #2696. The main difference is that this does not define a new resource and that it allows for multiple triggers instead of just one. I'm of the opinion that since the null_resource provisioner pattern is already used in codebases out in the wild that it makes sense to provide this as an optional parameter to the null_resource. That way the null_resource becomes a more fully featured interim solution before provisioners eventually get lifted up as first class citizens in the execution graph.

See: #580, #1181, #2677, #3080, #3220

Here's an example of how it could be used. This implementation happens to put a watch trigger on two computed values of an aws_instance. There isn't a reason to watch both other than to show off the syntax of how it could be used to trigger upon change of multiple resource values.

resource "aws_instance" "example" {
    ami = "ami-aa7ab6c2"
    instance_type = "t1.micro"
}

resource "null_resource" "trigger_test" {
    triggers {
        PRIVATE_IP = "${aws_instance.example.private}"
        PUBLIC_IP  = "${aws_instance.example.public_ip}"    
    }

    connection {
        user = "ubuntu"
        host = "${aws_instance.example.public_ip}"
        key_file = "~/.ssh/id_rsa"
    }

    provisioner "remote-exec" {
        inline = [
            "hostname"
        ]
    }

}

[apparentlymart]

Is it intended to eventually have standalone provisioners that don't belong to a resource? If so, I agree it's simpler to continue this hack of using null_resource; part of my intent in #2696 was to give it a more memorable and explainable name, but that's less important if it's only a temporary workaround.

My first version of stateful_provisioning has a map for the state, but I'd concluded that didn't really provide much value since you can't do anything with those values anyway. The only reason I could think of to make it a map rather than a string was that you might want to use {{self}} to access them in the provisioners:

resource "null_resource" "trigger_test" {
    triggers {
        build_id = "${var.build_id}"
    }

    connection {
        user = "ubuntu"
        host = "${aws_instance.example.public_ip}"
        key_file = "~/.ssh/id_rsa"
    }

    provisioner "remote-exec" {
        inline = [
            "do-some-deployment ${self.triggers.build_id}}"
        ]
    }
}

...but that seems like a rather questionable improvement over just using ${var.build_id} directly in the provisioner definition.

Via interpolations one can easily encode multiple things into the state_key string when needed, but I think the common case is to use a single value (like a build id in my case, or an instance id if in your case, or the URL of a deployment archive in a further case) and having multiple values would be rare.

No strong opinion either way really, but I'd settled on a single string just because it seemed simpler to explain to folks and covered the primary use-cases I had in mind.

[knuckolls]

You and I agree, I have no strong opinion either way. I wrote this up before I saw your PR and they're very similar indeed. There have been discussions about provisioners being first class citizens. I'm shooting for something like stateful_provisioning or this version of the null_resource with triggers to be merged so there's some official support for the feature. As things become sufficiently complex with Terraform this feature tends to come up.

[knuckolls]

@phinze we're using this same code as a plugin in our system so that we override the null resource. I'd prefer to not do that perpetually. Any thoughts on whether or not this is mergeable?

[knuckolls]

Bumping this thread in hopes to get it merged. For those interested, our plugin version of this PR can be found here: https://github.com/Banno/terraform-provider-null.

[phinze]

I see you @knuckolls 😀 - this is on my list for this week. 👍

[knuckolls]

Cool thanks!

[phinze]

Okay, finally got around to picking this and #2696 back up for review. Hello again! 😀 👋

First of all, thank you both for your efforts in implementation, documentation, and conversation around these features. The Terraform project is lucky to have such intelligent and capable contributors who are willing to donate their time towards making it better!

After re-reading the backlog on both of these PRs, here's my take on the various questions that are up in the air:

• We are indeed headed towards first class standalone provisioners, so I think @apparentlymart is right that this is the best next step - enhance null_resource with the idea that it will eventually be the basis of the implementation behind whatever becomes the top-level config.
• The triggers map is good, since is allows a bit more semantic clarity in both configs and diffs when there are two attributes that should trigger a provisioner rerun.
• The null_resource is enough of a crucial tool today that it should be documented even though it's a "stopgap feature"

So, given all that - I'm going to merge this, close #2696, and write up some docs for null_resource. I'll move forward with this plan, but I welcome your thoughts on all this @knuckolls and @apparentlymart. 👍

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.