asset: Add TerraformVariables asset to generated tfvar files

[yifan-gu]

• Move the tfvar definition under pkg/types/config
• Add a function that converts the installconfig and ignitions to terraform vars, some are based on installer/pkg/config: Support loading InstallConfig YAML #236

Next steps are:

• Use the ignition content directly in the tfvar file to launch cluster
• Clean up and trim the terraform tfvar definition (type Cluster struct).

[wking]

Can't you use GetInstallConfig for instead of these last seven lines?

[wking]

The file-retrieval through here could be more compact with something like:

contents := map[*asset.Asset][]string
for _, dependency := range []*asset.Asset{t.bootstrapIgnition, t.masterIgnition, t.workerIgnition}
  state, ok = parents[dependency]
  if !ok {
    return nil, fmt.Errorf("failed to find %T in parents", dependency)
  }
  for _, content := range state.Contents {
    contents[dependency] = append(contents[dependency], string(content.Data))
  }
}

cluster, err := config.ConvertInstallConfigToTFVar(installConfig, contents[t.bootstrapIgnition][0], contents[t.masterIgnition], contents[t.workerIgnition])

[yifan-gu]

sounds good

[wking]

I'd thought worker ignition files would be obsolete since #119, although it looks like we still have some local code for them. Do you know who's going to be responsible for them long-term?

[yifan-gu]

Talked with @abhinavdahiya , looks like we still need to pass the worker ignition to the operator.

[wking]

These are not types, so I'd like to keep them out of pkg/types/.... DefaultChannel should probably live in pkg/rhcos, the ignition paths should probably live in pkg/asset/ignition, etc.

And I think this shift is a really good time to drop the old YAML-specific strings and just write the Terraform with the same JSON properties that we use for YAML, although that will mean updating the consuming Terraform code (e.g. with tectonic_cluster_cidr -> podCIDR.

[yifan-gu]

@wking I'd like to do that in another separate PR, wdyt?

[wking]

I'd like to do that in another separate PR, wdyt?

Fine with me.

[crawford]

Looks like this belongs to an earlier commit.

[yifan-gu]

moved

[abhinavdahiya]

maybe DefaultChannel ?

[abhinavdahiya]

/lgtm
/hold
@yifan-gu /hold cancel whenever its lgtm from @crawford / @wking as they previously had comments.

[wking]

If it helps, my probably-stale-by-now attempt at InstallConfig -> Cluster conversion is here.

[wking]

A few nits, but I think the Count++ thing may be a bug that needs addressing before merge.

[wking]

These will panic if the CIDR is null, but I'm fine with punting a more robust IPNet.String() to future work.

[yifan-gu]

why CIDR will be null? they are not pointers.

[wking]

nit: I'd prefer handling these with chained if else statements, since that feels more idiomatic to me than a switch for boolean-comparison cases.

And it would be nice to error out if both cfg.Platform.AWS and cfg.Platform.Libvirt were non-nil, but I'm fine punting that to follow-up validation work.

[yifan-gu]

I think switch makes it easier to read the condition then changed if else when the if else block is big.
But I'm fine to change it.

[wking]

I think switch makes it easier to read the condition then chained if else when the if else block is big.

Split anyghing that big out into helper functions? But whatever, I'm fine with any of these options ;).

[wking]

I think we want to revisit this approach soon, but am fine with this for now. Cross-linking #264.

[wking]

I think if is more idiomatic here than a single-case switch.

[wking]

This is strange. I thought Worker.Count was the total number of workers (like here). In that case, I expect you want cluster.Worker.Count += nodePool.Count here (and similarly for master above). As you have it, cluster.Worker.Count is just going to come out to be len(cluster.Worker.NodePools).

[wking]

/lgtm
/hold cancel

[wking]

/lgtm cancel

Looks like you need the Name from a23d3e6 (#279). Rebase?

[wking]

@crawford seems to prefer inline title case for these:

$ git grep -A1 ' Name(' origin/master -- pkg/asset/ | head -n11
origin/master:pkg/asset/ignition/bootstrap.go:func (a *bootstrap) Name() string {
origin/master:pkg/asset/ignition/bootstrap.go-	return "Bootstrap Ignition Config"
--
origin/master:pkg/asset/ignition/master.go:func (a *master) Name() string {
origin/master:pkg/asset/ignition/master.go-	return "Master Ignition Config(s)"
--
origin/master:pkg/asset/ignition/testasset_test.go:func (a *testAsset) Name() string {
origin/master:pkg/asset/ignition/testasset_test.go-	return "Test Asset"
--
origin/master:pkg/asset/ignition/worker.go:func (a *worker) Name() string {
origin/master:pkg/asset/ignition/worker.go-	return "Worker Ignition Config"

Can we follow that pattern with:

// Name returns the human-friendly name of the asset.
func (t *TerraformVariables) Name() string {
  return "Terraform Variables"
}

[yifan-gu]

Updated

[wking]

/lgtm

[wking]

I still don't think we need a constant for this (folks who need it should just use the Name() method). But we can adjust that later if we care.

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, wking, yifan-gu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [abhinavdahiya,wking,yifan-gu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment