- Notifications
You must be signed in to change notification settings - Fork948
feat: Unify cli behavior for templates create and update#1385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedMay 11, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
|
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I really like this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why isprovisioner ->test.provisioner a good idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 ittest. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think at that point we'd figure out how to officially support it, and not re-use this flag.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@johnstcn left a great review, and LGTM too! 🥳
The change ofprovisioner totest.provisioner is really nice!
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: