[SEMVER-MAJOR] condition support to module params #350
Conversation
changes includes - removing global config - module config’s parameter to support condition - module config’s parameter to support custom typed prompts - prompts to not return value, instead update map(can support multi-value)
davidcheung
force-pushed
the
remove-credential-file
branch
from
0d6aec2 to
7713479
Compare
|
Go convention - can you please add comments on public functions starting with the name of the function? |
internal/apply/apply.go
Outdated
| }) | ||
| } | ||
|
|
||
| func getParameterDefinition(modConfig moduleconfig.ModuleConfig, field string) moduleconfig.Parameter { |
There was a problem hiding this comment.
Is this function not actually used anywhere?
There was a problem hiding this comment.
Yeah looks like it, will remove
|
Can you add a new file in docs to capture the schema and usage of the module.yml now that it's getting more complex? |
pkg/credentials/credentials.go
Outdated
| SecretAccessKey string `yaml:"secretAccessKey,omitempty" env:"AWS_SECRET_ACCESS_KEY,omitempty"` | ||
| } | ||
|
|
||
| var GetAWSCredsPath = awsCredsPath |
There was a problem hiding this comment.
Why assign here rather than calling the function directly?
There was a problem hiding this comment.
In the tests i'm overwriting the function so it fetches the mock path, it only lets me do it if its a variable
what's the proper way to
I guess I should just have the function receive the credentials path as well?
There was a problem hiding this comment.
Yeah how about you accept an optional base path, and if it's not supplied we default to the home dir?
pkg/credentials/credentials.go
Outdated
|
|
||
| func AwsCredsPath() string { | ||
| type AWSResourceConfig struct { | ||
| AccessKeyID string `yaml:"accessKeyId,omitempty" env:"AWS_ACCESS_KEY_ID,omitempty"` |
There was a problem hiding this comment.
Why "yaml"? If this annotation is just to be used by the ReflectStructValueIntoMap function we might as well use our own identifier so it's clear that it's not supposed to be used in some other way to generate yaml.
There was a problem hiding this comment.
No particular reason, mostly just because I reused the struct from before (global creds)
what should we change it to, should we just call it zero?
There was a problem hiding this comment.
How about just key or something? If we control this struct entirely in code and don't need to worry about parsing it to/from a file it might be a bit overkill to even use an annotation for this.
| | `data` | list(string) | Supply extra data for condition to run | | ||
|
|
||
|
|
||
| ### Validation |
There was a problem hiding this comment.
Missing info on some of the other things like the contitions and template fields related to templating.
There was a problem hiding this comment.
definitely good call to add this documentation, I looked at conditions and thought oh its there, but theres actually 2 types
There was a problem hiding this comment.
Yeah the naming might be a bit confusing..
internal/init/prompts.go
Outdated
| err, result = promptParameter(p) | ||
| } | ||
| if err != nil { | ||
| exit.Fatal("Exiting prompt: %v\n", err) |
There was a problem hiding this comment.
Mind adding a bit more info here? Maybe say that it's related to the module properties, and if possible, say which module the problem is with? I'm thinking for CustomPromptHandler for example, if the module specifies an invalid string, it's gonna be hard for a user to understand what's going on.
Co-authored-by: Bill Monkman <bmonkman@gmail.com>
Co-authored-by: Bill Monkman <bmonkman@gmail.com>
feature changes includes:
OmitFromProjectFiledefault's tofalse)behaviour changes includes: