fix: make template push a superset of template create #11299

f0ssel · 2023-12-20T13:48:26Z

The goal of this PR is to deprecate template create and have all options available for template push.

If the template doesn't exist it automatically creates the template now, the --create flag has been removed.
The new behavior is that after any calls to create template api (creating) or the template version api (updating) we will also call the edit api.
This means the coder templates push command now has all the flag options of both create and edit.
A deprecation notice has been added to coder templates create suggestion to now use push.
Although this is a superset of both create and edit now, because it still requires a new version to be pushed and coder template edit does not, we think we should keep them both for now.

Help:

➜  coder git:(f0ssel/templatepush) ✗ go run ./cmd/coder/main.go template push -h
coder v0.0.0-devel

USAGE:
  coder templates push [flags] [template]

  Push a new template version from the current directory or as specified by flag

OPTIONS:
      --deprecated string
          Sets the template as deprecated. Must be a message explaining why the template is deprecated.

      --activate bool (default: true)
          Whether the new template will be marked active.

      --allow-user-autostart bool (default: true)
          Allow users to configure autostart for workspaces on this template. This can only be disabled in enterprise.

      --allow-user-autostop bool (default: true)
          Allow users to customize the autostop TTL for workspaces on this template. This can only be disabled in enterprise.

      --allow-user-cancel-workspace-jobs bool (default: true)
          Allow users to cancel in-progress workspace jobs.

      --always-prompt bool
          Always prompt all parameters. Does not pull parameter values from active template version.

      --autostart-requirement-weekdays string-array
          Edit the template autostart requirement weekdays - workspaces created from this template can only autostart on the given
          weekdays. To unset this value for the template (and allow autostart on all days), pass 'all'.

      --default-ttl duration (default: 24h)
          Specify a default TTL for workspaces created from this template. It is the default time before shutdown - workspaces created
          from this template default to this value. Maps to "Default autostop" in the UI.

      --description string
          Edit the template description.

  -d, --directory string (default: .)
          Specify the directory to create from, use '-' to read tar from stdin.

      --display-name string
          Edit the template display name.

      --dormancy-auto-deletion duration (default: 0h)
          Specify a duration workspaces may be in the dormant state prior to being deleted. This licensed feature's default is 0h (off).
          Maps to "Dormancy Auto-Deletion" in the UI.

      --dormancy-threshold duration (default: 0h)
          Specify a duration workspaces may be inactive prior to being moved to the dormant state. This licensed feature's default is 0h
          (off). Maps to "Dormancy threshold" in the UI.

      --failure-ttl duration (default: 0h)
          Specify a failure TTL for workspaces created from this template. It is the amount of time after a failed "start" build before
          coder automatically schedules a "stop" build to cleanup.This licensed feature's default is 0h (off). Maps to "Failure cleanup"in
          the UI.

      --icon string
          Edit the template icon path.

      --ignore-lockfile bool (default: false)
          Ignore warnings about not having a .terraform.lock.hcl file present in the template.

      --max-ttl duration
          Edit the template maximum time before shutdown - workspaces created from this template must shutdown within the given duration
          after starting. This is an enterprise-only feature.

  -m, --message string
          Specify a message describing the changes in this version of the template. Messages longer than 72 characters will be displayed
          as truncated.

      --name string
          Specify a name for the new template version. It will be automatically generated if not provided.

      --private bool
          Disable the default behavior of granting template access to the 'everyone' group. The template permissions must be updated to
          allow non-admin users to use this template.

      --provisioner-tag string-array
          Specify a set of tags to target provisioner daemons.

      --require-active-version bool (default: false)
          Requires workspace builds to use the active template version. This setting does not apply to template admins. This is an
          enterprise-only feature.

      --var string-array
          Alias of --variable.

      --variable string-array
          Specify a set of values for Terraform-managed variables.

      --variables-file string
          Specify a file path with values for Terraform-managed variables.

  -y, --yes bool
          Bypass prompts.

f0ssel · 2023-12-20T14:17:38Z

Before I go too far into making tests work and cleaning up the PR, I'd like @sreya to take a look and make sure this approach is sound. Thanks

matifali · 2023-12-21T06:21:41Z

@f0ssel, this is great work. 👍🏼

This PR is a bit out of scope, but do you think we can also pull all template values, including metadata, using coder template pull or probably a new command coder template info? This can be a separate PR, but it would improve the UX to get the current values using the CLI before pushing an update. It would be even better to have a --output json output for this.

f0ssel · 2024-01-02T18:33:21Z

@matifali That all seems possible in a separate issue from my understanding but is out of scope for this PR. Can look into it next though.

matifali · 2024-01-02T19:31:30Z

@f0ssel sounds fine.

ammario

My hope with the original issue was that we would reduce overall code complexity and quantity. Do you see a path to doing that here?

Also, I'm surprised we're checking entitlements in the CLI. It seems like we could separate concerns (and simplify the code) by just checking for entitlements on the server?

ammario · 2024-01-02T19:13:31Z

cli/templatecreate.go

+			_, _ = fmt.Fprintln(inv.Stdout, "\n"+pretty.Sprint(cliui.DefaultStyles.Wrap,
+				pretty.Sprint(
+					cliui.DefaultStyles.Warn, "DEPRECATION WARNING: The `coder templates push` command should be used instead. This command will be removed in a future release. ")+"\n"))
+			time.Sleep(1 * time.Second)


Love the sleep-based incentive, but we should tell the user that it's sleeping for a moment in the deprecation message so that the user doesn't just think our CLI is slow.

ammario · 2024-01-02T19:19:01Z

cli/templatecreate.go

+				return xerrors.Errorf("get experiments: %w", exErr)
+			}
+
+			if !experiments.Enabled(codersdk.ExperimentWorkspaceActions) {


Aside: I've noticed us call this "WorkspaceActions" and "WorkspaceCleanup" in different places. E.g. here. @sreya can we standardize on one? Workspace Cleanup makes more sense to me.

ammario · 2024-01-02T19:20:44Z

cli/templateedit.go

-					}
-				}
-			}
-


Thanks for splitting out this code.

ammario · 2024-01-02T19:22:37Z

cli/templateedit.go

+				allowUserAutostop:              allowUserAutostop,
+				requireActiveVersion:           requireActiveVersion,
+				deprecationMessage:             deprecationMessage,
+			})


These mega structs are big code smells. It's not your PR's fault, but would enjoy seeing someone clean it up.

On second thought, why don't we use the codersdk types when validating entitlements?

ammario · 2024-01-02T19:23:52Z

cli/templateedit.go

+	requireActiveVersion           bool
+}
+
+func checkEditTemplateEntitlements(ctx context.Context, args checkEditTemplateEntitlementsArgs) (bool, error) {


A bit surprised that we can't reuse much of the code between checking edit and checking create entitlements?

ammario · 2024-01-02T19:25:36Z

cli/templateedit.go

+	disableEveryone                bool
+}
+
+func updateTemplateMetaRequest(args updateTemplateMetaArgs) codersdk.UpdateTemplateMeta {


See above comments on code reuse.

ammario · 2024-01-02T19:28:04Z

cli/templatepush.go

+		allowUserAutostart             bool
+		allowUserAutostop              bool
+		allowUserCancelWorkspaceJobs   bool
+		deprecationMessage             string


I would expect all these variable declarations to live in a struct somewhere so we weren't constantly repeating them.

ammario · 2024-01-02T19:29:10Z

cli/templatepush.go

@@ -188,10 +241,15 @@ func (r *RootCmd) templatePush() *clibase.Cmd {
 				return err
 			}

+			if utf8.RuneCountInString(name) > 31 {


Nit:

Suggested change

if utf8.RuneCountInString(name) > 31 {

if utf8.RuneCountInString(name) >= 32 {

less cognitive overhead to read the if statement and comment this way

f0ssel · 2024-01-02T19:51:15Z

@ammario Thanks for the review. There will be some code reduction once coder template create is removed, in the mean time it simplifies the create command at the cost of making the push command more complicated. If we feel this isn't a good solution I'm more than OK with scrapping this and going in a different direction, because I too am not super pleased with the lack of code reduction in this PR.

If API and code clarity are what we are really valuing here, I would almost suggest the opposite of this approach and make
"dumber" distinct template create|edit-metadata|update commands that don't do so much, have less overlapping flags, and requires the user to use each for the use case instead of a mega command like push that does everything. My understanding of the goal was really to deprecate create so we have one less command to maintain while giving users the simplicity of one command for any and all template updates. Could you give me some insight into which direction you would prefer here?

In addition to other comments I'll address, I can look into the entitlement stuff getting checked on the server, I also felt the issue with separation of concerns here.

ammario · 2024-01-02T20:15:27Z

A major perceived benefit of combining the commands was idempotency. For example, in CI, you could write a script that iterates all folders and runs coder templates push in them without consideration as to whether a template already exists.

I like to think the best solution for the user and the codebase is the same thing. As an aside, we mistakenly call a lot of these fields "metadata" when in fact there's nothing "meta" about it. We should instead call it the template configuration. metadata is typically read-write-able labels meant to be consumed by third party software.

What do think about combining template create into template push but then keeping a separate command for configuration. That configuration command could be template config, which could also print the configuration to stdout as well as enable changing it.

ammario · 2024-01-02T20:16:32Z

We call "config","metadata",whatever "settings" in the UI. So we should probably either follow that or change it everywhere.

f0ssel · 2024-01-03T15:09:38Z

A major perceived benefit of combining the commands was idempotency. For example, in CI, you could write a script that iterates all folders and runs coder templates push in them without consideration as to whether a template already exists.

@ammario So there's where the difficulty is in these changes lie - we want to support flags that the create API accepts, but we don't always use the template create API because it could just be a version update. In the case where we update, we need to then use the template edit API to apply those flags as a supplement. This is where the template edit code comes into the push command, even if we keep it as a separate command itself. So for push we are kind of left with these options:

Make push and create command "dumber" - Drop the flags that rely on the create / edit API for push altogether, only push new template versions. This is the existing behavior in main. We would then drop those settings flags on create and make it "dumber" but now a true subset of push.
Make push command "smarter" - Move all the create command flags into push command, use the edit API to apply those flags when not creating and only updating. This is how this current PR works. This makes push a superset of the existing create with all it's current "settings" flags. I've also added in the edit flags because why not - we are already calling the edit API. But these could be removed from this PR to make it slightly cleaner code.
Make push only apply "settings" flags on create - This is tricky behavior for the end user, because you would need to silently not apply those flag values to ensure idempotency because errors when supplied "settings" flags on non-create executions would mean you couldn't run the same script twice. This would be the "cleanest" code but not a very good UX imo.

All 3 of these template commands do pretty different things and rely on different APIs, so we just need to figure out where to unify them. My favorite is #1, which comes at the cost of a less useful create command but moves those concerns into the edit command and makes the cleanest API and code IMO. Would like to know your thoughts here as well.

Secondly, I looked and after removing the create command sometime in the future it makes the code diff lose about 800 loc, which then balances out this new additional code. Idk how soon of a timeline we remove it though without hurting existing customer usability.

f0ssel · 2024-01-03T16:15:38Z

I've mocked up a PR of what option 1 would look like here - #11390 basically we strip the settings flags from create and combine as much code as possible between the two commands for now. This would allow us to lead users to only using the push and edit|settings commands for all of their needs as well as removes the entitlements code required in create.

ammario · 2024-01-03T19:07:36Z

OK thanks for the explanation. I'll leave a comment within #11390.

f0ssel · 2024-01-04T18:15:43Z

Closing in favor of #11390

github-actions bot assigned f0ssel Dec 20, 2023

f0ssel requested a review from sreya December 20, 2023 14:16

sreya removed their request for review December 21, 2023 19:53

f0ssel force-pushed the f0ssel/templatepush branch from 982def3 to ff704ed Compare December 21, 2023 20:16

f0ssel marked this pull request as ready for review December 22, 2023 15:20

f0ssel requested review from ammario and sreya December 22, 2023 15:40

matifali mentioned this pull request Dec 22, 2023

Template pull mentions wrong flag for pulling the latest (non-active) version #11328

Closed

github-actions bot added the stale This issue is like stale bread. label Jan 2, 2024

matifali removed the stale This issue is like stale bread. label Jan 2, 2024

f0ssel added 18 commits January 2, 2024 18:23

fix: make template push a superset of template create

7ce08a1

add back work dir

675feb0

add groupacl

06618d7

Add back workdir

95d1a36

combine edit flags

7dd63a8

fix edit

4443322

unify edit and push

82b143b

make gen

19a4dfc

fix test

5a2aa6a

make gen

4ce4ddb

update golden

39ed2cd

fix tests:

2ad127b

fix merge

f715efc

add test

6269898

remove test

d1cf919

add unset test

6bcc688

Add deprecation warning

a5ac9b7

fix

132efda

f0ssel force-pushed the f0ssel/templatepush branch from 5934621 to 132efda Compare January 2, 2024 18:23

rename functions

9da9aed

ammario requested changes Jan 2, 2024

View reviewed changes

f0ssel closed this Jan 4, 2024

github-actions bot locked and limited conversation to collaborators Jan 4, 2024

github-actions bot deleted the f0ssel/templatepush branch July 3, 2024 00:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: make template push a superset of template create #11299

fix: make template push a superset of template create #11299

f0ssel commented Dec 20, 2023 •

edited

Loading

f0ssel commented Dec 20, 2023

matifali commented Dec 21, 2023 •

edited

Loading

f0ssel commented Jan 2, 2024

matifali commented Jan 2, 2024

ammario left a comment

ammario Jan 2, 2024

ammario Jan 2, 2024

ammario Jan 2, 2024

ammario Jan 2, 2024

ammario Jan 2, 2024

ammario Jan 2, 2024

ammario Jan 2, 2024

ammario Jan 2, 2024

ammario Jan 2, 2024

f0ssel commented Jan 2, 2024

ammario commented Jan 2, 2024

ammario commented Jan 2, 2024

f0ssel commented Jan 3, 2024

f0ssel commented Jan 3, 2024

ammario commented Jan 3, 2024

f0ssel commented Jan 4, 2024

	if utf8.RuneCountInString(name) > 31 {
	if utf8.RuneCountInString(name) >= 32 {

fix: make template push a superset of template create #11299

fix: make template push a superset of template create #11299

Conversation

f0ssel commented Dec 20, 2023 • edited Loading

f0ssel commented Dec 20, 2023

matifali commented Dec 21, 2023 • edited Loading

f0ssel commented Jan 2, 2024

matifali commented Jan 2, 2024

ammario left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f0ssel commented Jan 2, 2024

ammario commented Jan 2, 2024

ammario commented Jan 2, 2024

f0ssel commented Jan 3, 2024

f0ssel commented Jan 3, 2024

ammario commented Jan 3, 2024

f0ssel commented Jan 4, 2024

f0ssel commented Dec 20, 2023 •

edited

Loading

matifali commented Dec 21, 2023 •

edited

Loading