add package sylt to provide a workflow for lingon #120
Conversation
jlarfors
force-pushed
the
sylt
branch
2 times, most recently
from
a12bb7b
to
274c01c
Compare
There was a problem hiding this comment.
Did my best to comment on what I saw. Note that these are just my observations/opinions. All in all, it feels very object oriented with external state maintenance in sync. This is tradeoff where I don't really see or articulate the benefits, besides having to target an abstraction that is a little higher than the previous one. If it works for you, all good 😄.
| ActionName() string | ||
| // ActionType is the type of the action. | ||
| // It is called ActionType to be consistent with ActionName. | ||
| // All actions must define an action type. | ||
| ActionType() ActionType |
There was a problem hiding this comment.
This feels unnecessary. The only place it is used is in the logs and when adding a new action which in case of duplication. The type is known at compile time and can be accessed in many ways.
There was a problem hiding this comment.
Agree that it feel unnecessary right now, but if introducing some kind of middleware or other logic in the workflow based on the type of action, I would rather keep this and use it to do switch or if, than using reflect to check if a value is a type of a struct (or implements an interface).
| // Cleanup the action. | ||
| // It is used to cleanup after the action. | ||
| // It is only called if destroy is true. | ||
| Cleanup(ctx context.Context, opts RunOpts) error |
There was a problem hiding this comment.
The cleanup feels more and more like an edge case to me. I would not care about and I would prefer to handle manually as this is a sensitive operation. Having it in the interface feels premature. That is just my opinion.
| type RunOpts struct { | ||
| DryRun bool | ||
| Destroy bool |
There was a problem hiding this comment.
There are a few options that the user need to be set. Wouldn't it be safer to set a default dry-run to true ?
If you don't need to keep the Destroy option and the Cleanup method, it seems unnecessary to have a struct just for one boolean. This could be defined in another, less generic way than in RunOpts.
| terraMD5File = "tf.md5" | ||
| ) | ||
|
|
||
| const ActionTypeTerra ActionType = "terra" |
There was a problem hiding this comment.
The ActionType is used only once and is declared somewhere in another file. It is only used in the logs and the program would work the same by just having a string.
pkg/x/sylt/terra.go
Outdated
| type TerraActionOpts struct { | ||
| EnableCache bool | ||
| } |
There was a problem hiding this comment.
One bool in a struct is an unnecessary abstraction. Just use the bool directly.
| } | ||
|
|
||
| // Skip plan if the stack is cached. | ||
| var diff bool |
There was a problem hiding this comment.
could the diff be a real diff as in cmp.Diff ?
pkg/x/sylt/terra.go
Outdated
| if err := terra.Export(a.Stack, terra.WithExportWriter(&buf)); err != nil { | ||
| return false, fmt.Errorf("exporting stack: %w", err) | ||
| } | ||
| return compareLocalMD5Sum( |
There was a problem hiding this comment.
The code is used only once, maybe put it here directly. extract it when needed.
pkg/x/sylt/terra.go
Outdated
| if err := terra.Export(a.Stack, terra.WithExportWriter(&buf)); err != nil { | ||
| return fmt.Errorf("exporting stack: %w", err) | ||
| } | ||
| return writeLocalMD5Sum( |
There was a problem hiding this comment.
The code is used only once, maybe put it here directly. extract it when needed.
| for _, res := range sb.Resources { | ||
| resFound := false | ||
| for _, sr := range stateResources { |
There was a problem hiding this comment.
could it be that some resources in the state are not in the stack ?
There was a problem hiding this comment.
Yes, it is possible. If a plan&apply has not been performed and we are just checking the state, for example.
There was a problem hiding this comment.
Added a status for this, StateStatusOverflow. Perhaps not the best name but the best I could do right now.
| type WorkflowOption func(*workflowOpts) | ||
|
|
||
| // WithWorkflowDryRun sets the dry run option for Workflow. | ||
| func WithWorkflowDryRun(b bool) WorkflowOption { |
There was a problem hiding this comment.
bool is already false, no need to pass it unless it is true
There was a problem hiding this comment.
I like to pass bools to these, because when you go to write your CLI (or whatever will use it), it is much cleaner to just pass your bool, rather than doing an optional append of options... E.g.
opts := []SomeOption{}
if dryRun {
opts = append(opts, WithDryRun())
}vs
opts := []SomeOption{WithDryRun(dryRun)}I've had to do this enough that I just prefer it this way 🤷♂️
| } | ||
|
|
||
| // WithWorkflowDestroy sets the destroy option for Workflow. | ||
| func WithWorkflowDestroy(b bool) WorkflowOption { |
jlarfors
force-pushed
the
sylt
branch
2 times, most recently
from
f3a912f
to
d5d436d
Compare
Intial support for terra stacks only. Nesting under pkg/x/sylt to indicate that this is experimental.
Intial support for terra stacks only.
Nesting under pkg/x/sylt to indicate that this is experimental.