feat(terraform): add plugin commands for terraform apply and plan #1641

edvald · 2020-02-25T11:35:11Z

What this PR does / why we need it:

This makes the terraform provider a little easier to work with
day-to-day. We also explicitly discourage use of autoApply now, except
for private environments. The status check is now also faster, because
we don't refresh the stack state every time.

This involved a few refactors, mainly to improve the plugin command
framework. I also needed to modify the provider resolution flow so that
individual providers can be resolved (otherwise it wouldn't be possible
to run the apply/plan commands if another provider depended on the
outputs).

Special notes for your reviewer:

Please try using this with a real project, see if it matches your expectations,
and the interface+docs feel clear and sensible.

eysi09 · 2020-03-02T06:20:18Z

@solomonope, would be great to get your eyes on this as well,

garden-service/src/commands/plugins.ts

eysi09 · 2020-03-04T08:45:54Z

garden-service/src/commands/plugins.ts

@@ -34,6 +36,10 @@ export class PluginsCommand extends Command<Args> {
  name = "plugins"
  help = "Plugin-specific commands."

+  // FIXME: We need this while we're still resolving providers in the AnalyticsHandler


Do you know how we would fix this?

By not doing that, basically. We'd add metadata as it becomes available, and not force/rush the init.

eysi09 · 2020-03-04T08:54:54Z

garden-service/src/commands/plugins.ts

+
+    garden ${chalk.yellow("[global options]")} ${chalk.blueBright("<command>")} ${chalk.white("[args ...]")}
+
+  ${chalk.white.bold("PLUGIN COMMANDS")}


We could extract this and the stuff below to a helper function since we're doing basically the same in https://github.com/garden-io/garden/tree/master/garden-service/src/commands/options.ts

I notice that the table config is duplicated as well, although not part of this PR per se.

Actually on second glance there isn't that much common with the code, seems like I'd be going out of my way there a bit. The table generation there could use the new helper function though, I guess.

eysi09 · 2020-03-04T09:12:08Z

garden-service/src/garden.ts

+      }
+
+      // As an optimization, we return immediately if all requested providers are already resolved
+      const alreadyResolved = names.map((name) => this.resolvedProviders[name]).filter(Boolean)


A nitpick but the variable name sounds like a boolean, perhaps alreadyResolvedProviders. Makes the providers = alreadyResolved line below easier to read.

eysi09 · 2020-03-04T09:14:03Z

garden-service/src/logger/logger.ts

@@ -54,7 +54,7 @@ export class Logger extends LogNode {

  static initialize(config: LoggerConfig): Logger {
    if (Logger.instance) {
-      throw new InternalError("Logger already initialized", {})
+      return Logger.instance


Trying to think why we didn't just do this in the first place.

Yeah, same. I couldn't think of a good reason.

garden-service/src/plugins/terraform/commands.ts

eysi09 · 2020-03-04T09:18:52Z

garden-service/src/plugins/terraform/common.ts

  } else if (plan.exitCode === 2) {
    // No error but stack is not up-to-date
    logEntry.setWarn({ msg: "Not up-to-date" })
    if (autoApply) {
      // Trigger the prepareEnvironment handler
      return { ready: false, outputs: {} }
    } else {
-      logEntry.warn(noAutoApplyMsg)
+      logEntry.warn({ symbol: "warning", msg: noAutoApplyMsg(applyCommand) })
      const outputs = await getTfOutputs(log, tfVersion, root)
      return { ready: true, outputs }


If you have autoApply set to false, and have a Terraform module that's not been applied, e.g. because you've never "deployed" it before, Garden will print Already deployed to the terminal but fail when other modules reference its runtime output.

Not sure how to workaround this tbh.

A similar thing would happen at the provider level but at least there you don't get the Already deployed message which is a little misleading.

We may need to special-case the module behavior (as opposed to the provider behavior). I can look into it.

eysi09 · 2020-03-04T10:11:28Z

garden-service/src/task-graph.ts

@@ -850,11 +850,15 @@ export class TaskNodeBatch {
   */
  taskCached(result: TaskResult, depResults: TaskResult[]): boolean {
    const key = result.key
-    this.results[key] = result
+    if (this.remainingResultKeys.has(key)) {


Why are we changing the task graph? And could @thsig lend a pair of eyes?

Ah yes, I forgot to mention this in the commit. There was basically an error in the TaskGraph that caused old/unrelated task results to be lumped in the TaskResults object, that prompted an issue while I was testing all this. Could split that out into a separate commit.

eysi09 · 2020-03-04T10:13:01Z

garden-service/src/tasks/resolve-provider.ts

@@ -104,6 +104,12 @@ export class ResolveProviderTask extends BaseTask {
  async process(dependencyResults: TaskResults) {
    const resolvedProviders: Provider[] = Object.values(dependencyResults).map((result) => result && result.output)

+    // Return immediately if the provider has been previously resolved
+    const alreadyResolved = this.garden["resolvedProviders"][this.config.name]


Same as my comment above, looks like a boolean. Admittedly I'm not looking at this in VSCode with the type hints.

examples/terraform-gke/garden.yml

eysi09 · 2020-03-08T21:17:19Z

There are a few console log statements in there, also needs to be squashed.

This makes the terraform provider a little easier to work with day-to-day. We also explicitly discourage use of `autoApply` now, except for private environments. The status check is now also faster, because we don't refresh the stack state every time. This involved a few refactors, mainly to improve the plugin command framework. I also needed to modify the provider resolution flow so that individual providers can be resolved (otherwise it wouldn't be possible to run the apply/plan commands if another provider depended on the outputs).

edvald assigned eysi09 Feb 25, 2020

edvald force-pushed the terraform-commands branch 2 times, most recently from 84e95d6 to 6d5d97f Compare February 26, 2020 17:30

eysi09 reviewed Mar 4, 2020

View reviewed changes

edvald commented Mar 4, 2020

View reviewed changes

examples/terraform-gke/garden.yml Outdated Show resolved Hide resolved

snyk-bot mentioned this pull request Mar 4, 2020

[Snyk] Upgrade vuex from 3.0.1 to 3.1.2 qsays/garden#4

Open

edvald force-pushed the terraform-commands branch from 6d5d97f to 37b71ed Compare March 5, 2020 20:03

eysi09 approved these changes Mar 5, 2020

View reviewed changes

edvald force-pushed the terraform-commands branch from 37b71ed to 105af55 Compare March 5, 2020 21:04

eysi09 force-pushed the terraform-commands branch from 0e4717f to 9dfa229 Compare March 8, 2020 16:18

edvald added 2 commits March 9, 2020 16:51

fix(task-graph): don't include results from other batches in output

6a33137

edvald force-pushed the terraform-commands branch from 9dfa229 to 6fa9d8e Compare March 9, 2020 15:53

Merge branch 'master' into terraform-commands

2cd7279

edvald merged commit b4283dd into master Mar 10, 2020

edvald deleted the terraform-commands branch March 10, 2020 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(terraform): add plugin commands for terraform apply and plan #1641

feat(terraform): add plugin commands for terraform apply and plan #1641

edvald commented Feb 25, 2020

eysi09 commented Mar 2, 2020

eysi09 Mar 4, 2020

edvald Mar 4, 2020

eysi09 Mar 4, 2020

eysi09 Mar 4, 2020

edvald Mar 4, 2020

edvald Mar 4, 2020

eysi09 Mar 4, 2020 •

edited

eysi09 Mar 4, 2020

edvald Mar 4, 2020

eysi09 Mar 4, 2020

edvald Mar 4, 2020

eysi09 Mar 4, 2020

edvald Mar 4, 2020

eysi09 Mar 4, 2020

eysi09 commented Mar 8, 2020


		garden ${chalk.yellow("[global options]")} ${chalk.blueBright("<command>")} ${chalk.white("[args ...]")}

		${chalk.white.bold("PLUGIN COMMANDS")}

feat(terraform): add plugin commands for terraform apply and plan #1641

feat(terraform): add plugin commands for terraform apply and plan #1641

Conversation

edvald commented Feb 25, 2020

eysi09 commented Mar 2, 2020

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

eysi09 Mar 4, 2020 • edited

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

eysi09 commented Mar 8, 2020

eysi09 Mar 4, 2020 •

edited