feat: Unify cli behavior for templates create and update #1385
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1385 +/- ##
==========================================
+ Coverage 66.93% 67.32% +0.38%
==========================================
Files 288 288
Lines 18856 19105 +249
Branches 241 241
==========================================
+ Hits 12622 12862 +240
+ Misses 4944 4931 -13
- Partials 1290 1312 +22
Continue to review full report at Codecov.
|
|
|
||
| currentDirectory, _ := os.Getwd() | ||
| cmd.Flags().StringVarP(&directory, "directory", "d", currentDirectory, "Specify the directory to create from") | ||
| cmd.Flags().StringVarP(&provisioner, "test.provisioner", "", "terraform", "Customize the provisioner backend") |
There was a problem hiding this comment.
Why is provisioner -> test.provisioner a good idea?
There was a problem hiding this comment.
Also, is it possible / advisable to allow people to switch from one provisioner to another for a template that already exists?
There was a problem hiding this comment.
It's because this is not a flag that should be exposed to users (which is why it's hidden on the lines below). Naming it test. is a convention we could use throughout the cli when we need to modify functionality for test purposes, it also makes it very clear that these are not ordinary flags.
There was a problem hiding this comment.
I mean, I get that terraform is the only "real" provisioner at the moment, but that may not be true long term, and so you'd use that flag outside of testing purposes.
There was a problem hiding this comment.
I think at that point we'd figure out how to officially support it, and not re-use this flag.
This PR changes the
templates updatecommand to be more in line withtemplates create.Changes:
-dflag, same ascreateupdatedirectory, same ascreatecreateandupdatethe--providerflag is now called--test.providerand has no shorthand to improve clarityupdateOther considerations:
templates create .,tempaltes update .and by default infer thenamefrom the provided directory. Ultimately decided to keep the behavior as-is but am open to make the change if it seems like a good ideaFixes #565
Observations: