PR 1 for fogg #1
Conversation
Put the version in a variable and add a version command.
| log.Printf("touching %s", d) | ||
| _, e = dest.Create(d) | ||
| if e != nil { | ||
| return e |
There was a problem hiding this comment.
would it be worth errors.Wrap these to capture stacks/potentially easier debugging?
There was a problem hiding this comment.
Yeah that's a good idea. This is a large diff already, so I will add a follow-up task to handle that https://app.asana.com/0/335732894489412/723850669292560
There was a problem hiding this comment.
Yeah that's a good idea. This is a large diff already, so I will add a follow-up task to handle that https://app.asana.com/0/335732894489412/723850669292560
apply/apply.go
Outdated
| log.Printf("templating %s", d) | ||
| writer, err := dest.OpenFile(d, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0755) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
why panic vs return the error?
There was a problem hiding this comment.
Good question – I don't know why I did this, I will change it.
| AWSProfileProvider *string `json:"aws_profile_provider"` | ||
| AWSProviderVersion *string `json:"aws_provider_version,omitempty"` | ||
| AWSRegion *string `json:"aws_region"` // maybe rename to provider region | ||
| AWSRegions *[]string `json:"aws_regions"` // maybe rename to provider region |
There was a problem hiding this comment.
are these the allowed aws regions?
why *[]string and not just []string?
There was a problem hiding this comment.
Maybe there is a better way to do this, but I am doing this because it is optional. But I guess slices are reference types already so that is redundant. Will clean this up in a followup https://app.asana.com/0/335732894489412/723850669292561
| Components map[string]*Component `json:"components"` | ||
| } | ||
|
|
||
| type Component struct { |
There was a problem hiding this comment.
Should Account, Env and Component share an embedded type?
type Account struct {
TerraformConfiguration
}
type Env struct {
TerraformConfiguration
Components map[string]*Component
}
There was a problem hiding this comment.
I suspect that is the case, but wanted to get things working first. I guess we are at the point where that refactoring could make sense. I will follow up here https://app.asana.com/0/335732894489412/723850669292562
| } | ||
|
|
||
| func ReadConfig(f io.ReadCloser) (*Config, error) { | ||
| defer f.Close() |
There was a problem hiding this comment.
would that potentially be dangerous for the caller?
There was a problem hiding this comment.
Yeah possibly, though I have assumed that any method that takes a io.ReadCloser will close it. Perhaps this should be a io.Reader and let the caller close it. I will follow up here https://app.asana.com/0/335732894489412/723850669292563
| "github.com/spf13/afero" | ||
| ) | ||
|
|
||
| type account struct { |
There was a problem hiding this comment.
what's the difference between these and the config/config.go ones?
There was a problem hiding this comment.
The main difference is the semantics of what is required.
In the config there are a bunch of optional fields that will supplied by our inheritance rules.
My general idea here was to have a separate plan stage in which we apply all the rules. And then apply can be a dumb application of that plan. My hope is that this makes the code somewhat easier to test (because you can assert on the plan results without worrying about how they are applied).
| fmt.Printf("Version: %s\n", p.Version) | ||
| fmt.Println("Accounts:") | ||
| for name, account := range p.Accounts { | ||
| fmt.Printf("\t%s:\n", name) |
There was a problem hiding this comment.
should these be printing a string(yaml.Marshal())?
There was a problem hiding this comment.
Maybe. I wasn't really intending this to be yaml output and I think we should actually move more towards human readable for this. Make sense?
| return r | ||
| } | ||
|
|
||
| func resolveStringArray(def []string, override *[]string) []string { |
There was a problem hiding this comment.
i think *override can still be nil
There was a problem hiding this comment.
Ah yeah because slices are reference types. I will follow up here https://app.asana.com/0/335732894489412/723850669292564
|
I am going to go ahead and merge this so I can start picking off these pieces in smaller PRs. |
A bunch of stuff here, but would be good to get eyes on it.