Skip to content

Fold template create into template push #9318

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Tracked by #10759
ammario opened this issue Aug 24, 2023 · 12 comments · Fixed by #11390
Closed
Tracked by #10759

Fold template create into template push #9318

ammario opened this issue Aug 24, 2023 · 12 comments · Fixed by #11390
Assignees
Labels
s4 Internal bugs (e.g. test flakes), extreme edge cases, and bug risks

Comments

@ammario
Copy link
Member

ammario commented Aug 24, 2023

Push is supposed to be a superset of create, so we're carrying significant code debt. The slight mismatches in their functionality are mostly mistakes, e.g.:

  • create locally validates the template name whereas push does not
  • create accepts TTLs whereas push does not

We should make sure to deprecate create for a period of time, perhaps with an annoying time.Sleep. We should also change push to create a new template by default.

Bonus points for reducing duplication across template edit too. See #9319 for an example bug.

@ammario ammario added the s4 Internal bugs (e.g. test flakes), extreme edge cases, and bug risks label Aug 24, 2023
@cdr-bot cdr-bot bot added chore labels Aug 24, 2023
@matifali
Copy link
Member

matifali commented Aug 25, 2023

coder templates push --create does create the templates now if not exists. I guess you are talking about making that behavior default.

May what we can do is make coder templates create an alias of coder templates push --create after this refactoring has been done.

And yes merging this would be a good idea. There is still a plenty of duplication in these three commands.

@matifali
Copy link
Member

matifali commented Aug 25, 2023

Observations

On further researching on this, I have noticed a few issues,

coder template create

coder templates create --help
Usage: coder templates create [flags] [name]

Create a template from the current directory or as specified by flag

Options
      --default-ttl duration (default: 24h)
          Specify a default TTL for workspaces created from this template.

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

      --failure-ttl duration (default: 0h)
          Specify a failure TTL for workspaces created from this template. This
          licensed feature's default is 0h (off).

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

      --inactivity-ttl duration (default: 0h)
          Specify an inactivity TTL for workspaces created from this template.
          This licensed feature's default is 0h (off).

  -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.

      --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.

      --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.

---

coder template push

templates push --help
Usage: coder templates push [flags] [template]

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

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

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

      --create bool (default: false)
          Create the template if it does not exist.

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

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

  -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.

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

      --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.

---

coder template edit

coder templates edit --help
Usage: coder templates edit [flags] <template>

Edit the metadata of a template by name.

Options
      --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.

      --default-ttl duration
          Edit the template default time before shutdown - workspaces created
          from this template default to this value.

      --description string
          Edit the template description.

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

      --failure-ttl duration (default: 0h)
          Specify a failure TTL for workspaces created from this template. This
          licensed feature's default is 0h (off).

      --icon string
          Edit the template icon path.

      --inactivity-ttl duration (default: 0h)
          Specify an inactivity TTL for workspaces created from this template.
          This licensed feature's default is 0h (off).

      --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.

      --name string
          Edit the template name.

  -y, --yes bool
          Bypass prompts.

---

Common

We have 2 types of properties for each template

  1. Specific to a template-id and independent of versions like
    • metadata, e.g., --icon, --display-name, --description
    • TTL settings, e.g. --max-ttl, --default-ttl, --failure-ttl, --inactivity-ttl
    • Permission settings, e.g., --allow-user-autostart, --allow-user-autostop, --allow-user-cancel-workspace-jobs, --private
  2. Specific to the template version
    • --name version name
    • --message changelog messages
    • --activate to mark as activate (default=true)

Issues

  1. coder template create uses name for the identifier of the template while coder template push uses template [First line]
  2. coder template create is missing specifying a custom template version --name and following flags --default-ttl,--failure-ttl, --ignore-lockfile, --inactivity-ttl and --private.
  3. Both coder template push and create are missing some metadata set flags. e.g., --description, --display-name, and '-icon'.
  4. Both coder template push and create are missing setting flags. e.g., --allow-user-autostart, --allow-user-autostop, and --allow-user-cancel-workspace-jobs
  5. coder template edit uses --name to edit the template identifier while coder template push uses the same flag to specify a version name

Suggested Solution

  1. Synchronise coder template create|push|edit
  2. I think the following should be used as a template identifier.
--id 		for the template unique identifier (We can add this flag to specify a template explicitly)
--name		for the template version name (also) unique (This is the current behavior)
--message	template version change messages (This is the current behavior)

All of them should be reflected in coder templates create|push --id my-template --name PyTorch2p0 --message "Updated PyTorch to version 2.0"

@matifali
Copy link
Member

@bpmct for comments

@bpmct
Copy link
Member

bpmct commented Aug 25, 2023

Hey, really appreciate you sharing this! I'm in favor of your proposal but think we should be careful with the naming. I'd rather users specify the template name as an argument, not a flag.

How about this?

coder templates create|push <my-template> --version "PyTorch2p0" --message "Updated PyTorch to version 2.0"

Main reason being we don't really call the template name the "ID" anywhere.

@matifali
Copy link
Member

@bpmct It won't hurt if can call this id as it is an identifier, or we should rename the version name from --name to something else to avoid confusion. Specifically point 5.

coder template edit uses --name to edit the template identifier, while coder template push uses the same flag to specify a version name

@bpmct
Copy link
Member

bpmct commented Sep 1, 2023

@matifali explained that we call the template name:

Screenshot 2023-09-01 at 9 32 45 AM

an id in the code and our endpoints accept both a uuid or id (name). I agree we should consolidate the naming across the dashboard, CLI, and API but I'm hesitant to break the CLI by making it so people have to reference it with `--id=my-template. If possible, I'd prefer to stay with this format

coder templates create|push <my-template> --version "PyTorch2p0" --message "Updated PyTorch to version 2.0"

@f0ssel
Copy link
Contributor

f0ssel commented Nov 27, 2023

We should make sure to deprecate create for a period of time, perhaps with an annoying time.Sleep.

lol I like it

@ammario
Copy link
Member Author

ammario commented Dec 11, 2023

What happened with this issue?

@f0ssel
Copy link
Contributor

f0ssel commented Dec 11, 2023

I had taken on too much for my first sprint and this was one of the items taken off after meeting with @sreya about it.

@f0ssel
Copy link
Contributor

f0ssel commented Dec 18, 2023

@matifali @bpmct Thanks for documenting the research here. I've looked at the code a little and I see why things are the way they are. Mainly:

  • template edit does not create a new active version, just edits metadata, doesn't require you provide a template directory
  • template create creates a new active version, creates new template where you can specify some metadata, requires you provide a template directory
  • template push creates a new active version, conditionally creates a new template, requires you provide a template directory

So you run into problems with combining these behaviors into a single template push:

  1. Previous users of template edit will now have to provide a template directory if they want to update meta data and this will make an additional template version for no reason. I'm not sure how to cleanly avoid this behavior in a single command without complicated branching that would suggest that it should be 2 separate commands. We should maybe consider keeping the metadata template edit command separate if we don't like the behavior I've described.
  2. Should the --create flag should continue to exist on template push or should the behavior be automatic?
  3. If we are not creating a new template but are provided metadata flag values, we will need to make an additional api call to edit the template before/after we make a new template version. If we are creating it for the first time we can provide some of those values I believe. Not the biggest deal really, but want to call out because there's no good way to do this transactionally and could leave users in an unclear state in a partial failure case.

I'm going to continue digging, but want to start the conversation around those points that I've noticed so far.

@matifali
Copy link
Member

  1. Should the --create flag should continue to exist on template push or should the behavior be automatic?

If we get rid of the template create in favor of template push, then yes, --create should be the default behavior if no previous version exists.

  1. Previous users of template edit will now have to provide a template directory if they want to update meta data and this will make an additional template version for no reason. I'm not sure how to cleanly avoid this behavior in a single command without complicated branching that would suggest that it should be 2 separate commands. We should maybe consider keeping the metadata template edit command separate if we don't like the behavior I've described.

I am okay with keeping it as a separate command that only edits/adds metadata, but it would be nice to have all of these options as part of the templates push, too. The idea is to create a template and set all metadata fields with a single command.

Also, we may rename templates edit to something like template update-metadata or template metadata. edit sounds like it is editing more than just metadata.

@f0ssel
Copy link
Contributor

f0ssel commented Dec 20, 2023

@matifali @bpmct @ammario Okay I have some usability questions for product - basically there's some difficulty with working with template edit api that is making this difficult. I have 2 clear paths I can go down:

  1. Make the flags like --default-ttl, etc (flags meant for just the create templates API) only work on template creation, and fail on template update if provided.
  2. Make the flags work on create and update (using the template edit api on updates, more work + code to test)

I originally started with 2 but thought I ask if 1 works from a product perspective while I continue down path 2. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s4 Internal bugs (e.g. test flakes), extreme edge cases, and bug risks
Projects
None yet
4 participants