Make things more godiomatic #67
Labels
code quality
Improvements to the internals that may not have user-facing impact
Comments
ErinCall
added
the
code quality
ErinCall
added a commit
that referenced
this issue
Jan 17, 2020
I'd like to be able to make calls like NewUpgrade(cfg) rather than
Upgrade{...}.Prepare, but I wouldn't be able to define a NewUpgrade
function while Config is in the helm package; there would be a circular
import when Plan tried to import run.
ErinCall
added a commit
that referenced
this issue
Jan 17, 2020
This seems to be be a more natural separation of concerns--the knowledge of which config fields map to which parts of a Step belong to the Step, not to the Plan.
ErinCall
added a commit
that referenced
this issue
Jan 17, 2020
Now that InitKube, AddRepo, and UpdateDependencies are initialized with NewSTEPNAME functions, the helper functions in plan.go are unnecessary--they do too little to be a useful abstraction, and they aren't complex or frequently-used enough to be worth extracting.
ErinCall
added a commit
that referenced
this issue
Jan 17, 2020
Now that the InitKube initialization happens inside its own package, the private .values field can be populated at the same time, rather than having to wait for Prepare(). Also clarified the config/template filename fields (configFile vs. ConfigFile was particularly ambiguous).
ErinCall
added a commit
that referenced
this issue
Jan 17, 2020
This is the first step toward removing run.Config entirely. InitKube was the only Step that even used cfg in its Execute function; the rest just discarded it.
ErinCall
added a commit
that referenced
this issue
Jan 17, 2020
(Just something I happened across while writing the previous commit)
ErinCall
added a commit
that referenced
this issue
Jan 17, 2020
This fixes the run package's leaky abstraction; other packages no longer need to know or care that run.Config even exists. Note that since the various Steps now depend on having a non-nil pointer to a run.Config, it's unsafe (or at least risky) to initialize them directly. They should be created with their NewSTEPNAME functions. All their fields are now private, to reflect this.
ErinCall
added a commit
that referenced
this issue
Jan 17, 2020
This is a general-purpose cleanup commit; every step except InitKube had the same six "add the --debug and --namespace flags if applicable" code.
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The current design is pretty idiosyncratic—it makes total sense to me, the person who wrote it, but someone coming in for the first time will probably find it bewildering.
There should probably be
New${StepName}()functions for the various Steps; they should either take the global config as an argument or use the.With${OptionName}style.The whole Step/Plan divide may not even be worthwhile; I wrote it in a mindset more appropriate to a large project with ongoing new-feature development.
The text was updated successfully, but these errors were encountered: