ashish-amarnath / capi-yaml-gen Goto Github PK

I think it would be good, if the user picks aws provider, to show a message saying

Environment is not configured properly, please set

• CONTROL_PLANE_INSTANCE_TYPE
• SSH_KEY_NAME

And then perhaps even defaults to copy paste like

export CONTROL_PLANE_INSTANCE_TYPE=t2.medium
export SSH_KEY_NAME=default

and then a way to avoid this error such as --allow-unset-env-vars (-f?)

We should start thinking about scenarios and how to get things like cloud provider flags piped into the right places.

If I am using kubeadm and i ask for the aws infrastructure provider, I should see all the required cloud-provider: aws flags in all the right places as well as a default secret I could fill in. In addition, we'd also need sshkey, iaminstance profile and instance size variables. I'm not sure a great way to get these values. But I think something like some YAML that can be run through envsubst would be great. Or perhaps we can accept a configuration file and output valid YAML? Perhaps both if no configuration is defined?

I'm not entirely sure how it would work yet as you can see, but this would be the start of something very useful imo.

At the very least we can put in the hardcoded defaults for AWS.

This should also support MachineDeployments + templates.

Not sure what the API for that will look like...

Maybe it's just a flag

--machine-deployment - Produces a machine deployment for worker nodes instead of raw Machines.

Right now it's a bit onerous to add a new provider. I think we can make interfaces for the various types and then grab the right struct satisfying the interface.

What if we had, for each provider, a struct like this:

package aws
type Generator struct {}
func (g *Generator) GetInfraMachineYAML() {}
func (g *Generator) GetInfraClusterYAML() {}
then in the cmd package we can define some interfaces like:

type InfrastructureProvider interface {
GetInfraMachineYAML()
GetInfraClusterYAML()
}

type BootstrapProvider interface {
GetConfig()
}

type CoreProvider Interface{
GetMachineYAML() {}
GetClusterYAML() {}
}
Then we can do a switch like

var infraProvider InfrastructurePRovider
switch p.InfraProvider {
case "aws"
infraProvider = &capa.Generator{}
...
}
and we'll always be able to call the same method regardless of provider.

It could be really useful to be able to go fetch the CRDs/manager deployment from this tool as well...WDYT?

With no unit tests, it is difficult to verify new changes don't unintentionally break yaml.

Add unit tests as follows:

1. Operating under the premise that current master HEAD is golden, create a dir, say test/golden/ generate yaml for all provider combinations and add it to the repo.
2. Write generate_test.go that will call runGenerateCommand with necessary options, capture output and compare w/ yaml in test/golden.
3. if the yaml files match, then the change is good. If the yaml files don't match, then there is an unintended change to the generated yaml or the golden files need to be updated.

The provider abstraction is still not quite there.

The providers should not have to know about the generator.Object type. This means we'll need to add one more layer to the abstraction.

We will need something that can wrap up the actual providers and keep that logic out of the generate.go file.

Something like this:

// generate.go
machineFactory, err := generator.NewMachineFactory(provider)
machine := machineFactory.Machine(name, namespace)

// generator.go

type Object interface {...}

// MachineFactory
type Factory struct {
  provider string
}

func NewFactory(provider string) (Factory, error) {
 // if provider is unknown return an error otherwise return a factory
}

func (f Factory) Machine(name, namespace string) Object {
  switch f.provider
  case "aws":
    capa.GenerateMachine()
  case "docker":
    capd.GenerateMachine()
  ...
}

// capd.go

// Notice the return type, no longer referencing generator.Object
func (p Provider) GenerateMachine(...arguments...) *infrav1.Machine {}

This would also negate the need for the Provider shell struct. The factory can just call the correct function from the correct package.

I suggest we add a branch or maybe a tag for v1alpha2 generation and then move master along to v1alpha3. I've got a PR i'm about to open to support v1alpha3 but I know for a fact i'll need v1alpha2 support as well and need to switch between the versions

this should not print status to objects.

If necessary we could hide it behind a flag, but I can't think of a use case right now that would ever necessitate needing the status object.