Skip to content
This repository has been archived by the owner on Jun 29, 2022. It is now read-only.

Refactor Terraform executor #794

Merged
merged 3 commits into from
Aug 14, 2020
Merged

Refactor Terraform executor #794

merged 3 commits into from
Aug 14, 2020

Conversation

johananl
Copy link
Member

This PR paves the way towards #716. By making terraform.Executor accept an arbitrary series of "steps" we can now declaratively define the infrastructure pieces a platform is composed of.

Example:

steps := []terraform.ExecutionStep{
    {
        Description: "create controllers",
        Args: []string{
            "apply",
            "-auto-approve",
            fmt.Sprintf("-target=module.packet-%s.packet_device.controllers", c.ClusterName),
        },
    },
    {
        Description: "create DNS records",
        Args:        []string{"apply", "-auto-approve", "-target=module.dns"},
    },
    {
        Description: "refresh Terraform state",
        Args:        []string{"refresh"},
    },
    {
        Description:      "complete infrastructure creation",
        Args:             []string{"apply", "-auto-approve"},
        // Verify DNS entries were manually created by the user before proceeding.
        PreExecutionHook: dns.ManualConfigPrompt(&c.DNS),
    },
}

I've deliberately avoided refactoring all the various platforms to keep the scope of this PR narrow. I've only refactored platform/packet (which is our most complex platform implementation at the moment) to demonstrate how the new structure can be used. The rest of the platforms will be refactored in subsequent PRs.

pkg/dns/dns.go Outdated Show resolved Hide resolved
@johananl johananl force-pushed the johananl/refactor-tf-executor branch 2 times, most recently from e45db75 to e82451b Compare August 11, 2020 19:23
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice PR @johananl 👍

pkg/terraform/executor.go Outdated Show resolved Hide resolved
pkg/dns/dns.go Show resolved Hide resolved
@johananl johananl force-pushed the johananl/refactor-tf-executor branch from e82451b to 7d33836 Compare August 12, 2020 11:15
@johananl johananl requested a review from invidian August 12, 2020 11:15
invidian
invidian previously approved these changes Aug 12, 2020
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though it is possible to do even more fair-grained commits, for example:

  • pkg/dns: handle stdin read errors
  • pkg/terraform: add OutputBytes function (to unexport executeSync)
  • pkg/dns: use terraform.OutputBytes instead of terraform.ExecuteSync
  • pkg/terraform: add ExecutionStep concept
  • pkg/dns: add ManualConfigPrompt function
  • pkg/platform/packet: switch to use terraform.ExecutionStep
  • pkg/terraform: unexport low-level functions

I added comments regarding some nits, but they are not very important for the scope of this PR.

pkg/dns/dns.go Show resolved Hide resolved
pkg/dns/dns.go Outdated Show resolved Hide resolved
pkg/dns/dns.go Show resolved Hide resolved
- Define a new type terraform.ExecutionStep which allows describing
  arbitrary Terraform operations declaratively.
- Allow passing one or more ExecutionSteps to the Terraform executor.

This allows making pkg/platform generic as now every platform
implementation can define its steps for deploying infrastructure in a
declarative way, thus making the code which interacts with Terraform
generic. Thanks to the PreExecutionHook concept, the same is true even
when arbitrary logic needs to be executed before some Terraform
operation: A platform implementation can include a callback function
in the PreExecutionHook field of an ExecutionStep and the generic
Terraform code will invoke it at the right time.
fmt.Scanln() returns an error which we should handle. fmt.Scanln()
returns an error not only when there was a problem reading from
stdin, but also when a newline is returned on its own, i.e. when the
user hits Enter without typing "skip". Since we rely on the user
hitting Enter for looping here, in order to distinguish between an
actual error and the user hitting Enter, we would have to check for
the sentinel value "unexpected newline" which isn't exported as a
constant by the fmt package.

By using ReadString() we can handle read errors simply without
treating newlines as an error.
ExecuteSync() and ExecuteAsync() are low level functions and are used
mainly internally by pkg/terraform.

Add a dedicated Output() method to the Executor API. Callers should
not have to allow on low-level functions to get outputs.
Copy link
Member

@invidian invidian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@surajssd surajssd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@johananl johananl merged commit 48a5afd into master Aug 14, 2020
@johananl johananl deleted the johananl/refactor-tf-executor branch August 14, 2020 13:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants