feat: Allow inheriting parameters from previous template_versions when updating a template #2397

Emyrk · 2022-06-16T00:08:39Z

What this does

The main goal of this PR is to prompt the user for new params if they update a template version with new parameters.

How it does this

I looked into a few methods, and honestly it was more complicated than it should have been. The current solution I'm not 100% satisfied with.

The current CLI flow for creating a template is this:

Upload the tarball as a file
Attempt to create the template version without prompting for params
- Since this almost always fails (as we need params) an empty template version now exists in the db
Read missing params error
Prompt missing params
Attempt create template version again with params
Create template for the said template version

So this process ends up with 2 template versions, 1 of which is never used. The reason this happens is because when you make a template version, the job that runs against the provisioner is async, so we don't know what params we are missing. Add ontop of this that param values should not be returned (or at least secret ones), it makes this new feature have a few restrictions.

Implementation

The current implementation adds a field to CreateParameterRequest called CopyFromParameter which is a uuid that points to another parameter. A list of CreateParameterRequests are sent to the backend with the template_version create.

The CLI then when finding it is missing parameters:

Query the current active template version.
Find it's params in the import_job scope (and only that scope)
Use CopyFromParameter for the missing params.
Prompt for any additional params (like newly added ones)

The backend then can fetch the parameter from CopyFromParameter and check a few things:

Both template_versions share the same template_id (cannot import from other templtes)
Scope is import_job

Alternatives tossed

Retrieve the param values and create the param requests

Secrets should never be readable. So any secrets cannot be pulled to be reinserted this way.

The ui/cli can not therefore "prepopulate" any param fields from the past params. At best we can say "previous value exists", but no always retrieve the value.

Imply inheriting parameters automatically

Another idea is if someone submits a template version that is missing params, the backend can automatically pull those params from the previous active version.

This falls apart. The param missing is checked in the provisioner, which does not have database access. So the job has to fail, and a new job has to be made with the params, which causes 2 job ids for 1 template version? It gets complicated.

The cli/ui is the actor watching the job failure and knows which params are missing. For the backend to implement this, the backend will have to watch for the job failure and act on that. We don't have this async generic job scheduler in the backend, so the implementation seems complicated at best.

Having the cli pass some "inherit all params" indicator, rather than a `CopyFromParameter` for each needed param

Because the job is async, the backend doesn't know which params are missing when creating a template version. So to do this, we'd always have to inherit all params, even if the params are excessive. This would cause extra params in the db that are no longer defined in the terraform.

- Allow more columns on template list

…_update_params

Emyrk · 2022-06-16T00:10:04Z

coderd/database/databasefake/databasefake.go

@@ -703,17 +716,26 @@ func (q *fakeQuerier) GetOrganizationsByUserID(_ context.Context, userID uuid.UU
 	return organizations, nil
 }

-func (q *fakeQuerier) GetParameterValuesByScope(_ context.Context, arg database.GetParameterValuesByScopeParams) ([]database.ParameterValue, error) {
+func (q *fakeQuerier) ParameterValues(_ context.Context, arg database.ParameterValuesParams) ([]database.ParameterValue, error) {


Made 1 query with filter options rather than adding new specific queries

johnstcn

I'm no expert on the params stuff, but I think this looks solid.

johnstcn · 2022-06-16T14:45:42Z

cli/templatecreate.go

+	// ReuseParams will attempt to reuse params from the Template field
+	// before prompting the user. Set to false to always prompt for param
+	// values.
+	ReuseParams bool


👍 making this opt-in

I plan to add some kind of --always-prompt flag if you want to change the params.

johnstcn · 2022-06-16T14:46:07Z

cli/templatecreate.go

+	ReuseParams bool
+}
+
+func createValidTemplateVersion(cmd *cobra.Command, args createValidTemplateVersionArgs, parameters ...codersdk.CreateParameterRequest) (*codersdk.TemplateVersion, []codersdk.CreateParameterRequest, error) {


yeah we're there now :D (args struct)

I really want to try refactoring this function. It's growing limbs and becoming quite large lol.

"Get it working" was my first step...

johnstcn · 2022-06-16T14:47:17Z

cli/templatecreate.go

+	// lastParameterValues are pulled from the current active template version if
+	// templateID is provided. This allows pulling params from the last
+	// version instead of prompting if we are updating template versions.
+	lastParameterValues := make(map[string]codersdk.Parameter)
+	if args.ReuseParams && args.Template != nil {
+		activeVersion, err := client.TemplateVersion(cmd.Context(), args.Template.ActiveVersionID)
+		if err != nil {
+			return nil, nil, xerrors.Errorf("Fetch current active template version: %w", err)
+		}
+
+		// We don't want to compute the params, we only want to copy from this scope
+		values, err := client.Parameters(cmd.Context(), codersdk.ParameterImportJob, activeVersion.Job.ID)
+		if err != nil {
+			return nil, nil, xerrors.Errorf("Fetch previous version parameters: %w", err)
+		}
+		for _, value := range values {
+			lastParameterValues[value.Name] = value
+		}
+	}
+


Question (non-blocking): is the "currently active" template version always guaranteed to be the "last" one?
Either way, I think this is probably the expected behaviour; you want to just re-use what you already have.

No it's not always the last one. This is actually intentional, as if you upload a template_version that fails, it won't become the "active" template, but it will be the "last" template.

So the "active" template is usually the last working template version

coderd/database/databasefake/databasefake.go

johnstcn · 2022-06-16T14:49:32Z

coderd/templateversions.go

+				// This is a bit inefficient, as we make a new db call for each
+				// param.
+				version, err := db.GetTemplateVersionByJobID(r.Context(), copy.ScopeID)


batch fetch params?

Yea, I can make this change if this method is ok with the team

Going to punt on this. We only have a handful of params right now. If it's a problem I can optimize later

cli/templatecreate.go

kylecarbs · 2022-06-16T19:07:36Z

coderd/templateversions.go

+		inherits := make([]uuid.UUID, 0)
 		for _, parameterValue := range req.ParameterValues {
+			if parameterValue.CopyFromParameter != uuid.Nil {
+				inherits = append(inherits, parameterValue.CopyFromParameter)
+			}
+		}


Could we just inherit by default if not provided? Then we wouldn't need CopyFromParameter, and I believe we could delete a buncha this!

I wanted to go down this route first, but it becomes really challenging in the backend to do this cleanly. The missing parameters are identified in the provisioner in the async job.

So the BE creates the template_version, and returns the cli. The CLI watches the job, reads the missing params error, then fills in the missing params.

To do this inheritance by default in the backend, either:

The provisioner daemon would need to fetch the current active template version to find the missing params during the import_job.

Some backend go routine/process would need to watch the import_job, identify the failure, and rerun the job with updated inherited params.

If I decide to just "always inherit all params" from the last version, then you end up with excessive params. Eg: if the new template version dropped params "a" and "b", it would still inherit these values. These excessive values just seem like a bug.

The implementation I came up with tracks the correct params. Deleted params from the terraform file are not carried forward. New params are prompted to the user.

…_update_params

kylecarbs

I haven't had time to run through the CLI experience, but this LGTM!

kylecarbs · 2022-06-17T13:37:55Z

codersdk/parameters.go

+	// succeed.
+	// No other fields are required if using this, as all fields will be copied
+	// from the other parameter.
+	CopyFromParameter uuid.UUID `json:"copy_from_parameter,omitempty" validate:"uuid4"`


What do you think about CopyFrom instead? Or CloneID? No specific reason, just preference.

CloneID I like.

kylecarbs · 2022-06-17T13:40:01Z

cli/templatecreate.go

@@ -195,14 +241,22 @@ func createValidTemplateVersion(cmd *cobra.Command, client *codersdk.Client, org

 		// parameterMapFromFile can be nil if parameter file is not specified
 		var parameterMapFromFile map[string]string
-		if parameterFile != "" {
+		if args.ParameterFile != "" {


I think we should remove the ParameterFile in favor of a --var=asd=asd approach. Separate issue, but I think it conflates some of this logic.

The file logic needs to be looked at for sure. I kept it as is for now.

Emyrk added 7 commits June 14, 2022 10:45

WIP: feat: Update templates also updates parameters

ed33a1c

Insert params for template version update

1f2892c

feat: Implement parameters list

8d62c96

- Allow more columns on template list

Working implementation of inherited params

aceca62

Merge remote-tracking branch 'origin/main' into stevenmasley/template…

effbde7

…_update_params

Fix name of enum

e569f0f

Lint cleanup

2e5bf44

Emyrk commented Jun 16, 2022

View reviewed changes

Emyrk added 4 commits June 15, 2022 19:24

Enable always prompting of params

e5b90bc

Fix db query for scope enum

9d17dac

Update comment

04098c3

Fix list parameters

3422844

johnstcn reviewed Jun 16, 2022

View reviewed changes

kylecarbs reviewed Jun 16, 2022

View reviewed changes

Emyrk requested a review from kylecarbs June 16, 2022 20:09

Emyrk added 2 commits June 16, 2022 15:18

Rename field

5f881aa

Merge remote-tracking branch 'origin/main' into stevenmasley/template…

bcca07a

…_update_params

kylecarbs approved these changes Jun 17, 2022

View reviewed changes

Emyrk added 2 commits June 17, 2022 09:37

Unit test param prompting

c43aedc

Add "--always-prompt" flag and logging info

fa9b230

Emyrk marked this pull request as ready for review June 17, 2022 15:00

Emyrk requested a review from a team as a code owner June 17, 2022 15:00

Emyrk added 3 commits June 17, 2022 10:14

Linting fixes

9438db8

Linting

5c590ae

Remove excessive prompt

f46b4af

Emyrk merged commit 64b92ee into main Jun 17, 2022

Emyrk deleted the stevenmasley/template_update_params branch June 17, 2022 17:22

Emyrk mentioned this pull request Jun 17, 2022

Edit template variables after creation #1886

Closed

kylecarbs mentioned this pull request Jun 22, 2022

templates with parameter cannot be updated #2570

Closed

Emyrk changed the title ~~WIP: Allow inheriting parameters from previous template_versions when updating a template~~ feat: Allow inheriting parameters from previous template_versions when updating a template Jun 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow inheriting parameters from previous template_versions when updating a template #2397

feat: Allow inheriting parameters from previous template_versions when updating a template #2397

Emyrk commented Jun 16, 2022 •

edited

Loading

Emyrk Jun 16, 2022

johnstcn left a comment

johnstcn Jun 16, 2022

Emyrk Jun 16, 2022

johnstcn Jun 16, 2022 •

edited

Loading

Emyrk Jun 16, 2022

johnstcn Jun 16, 2022

Emyrk Jun 16, 2022

johnstcn Jun 16, 2022

Emyrk Jun 16, 2022

Emyrk Jun 17, 2022

kylecarbs Jun 16, 2022

Emyrk Jun 16, 2022

kylecarbs left a comment

kylecarbs Jun 17, 2022

Emyrk Jun 17, 2022

kylecarbs Jun 17, 2022

Emyrk Jun 17, 2022

feat: Allow inheriting parameters from previous template_versions when updating a template #2397

feat: Allow inheriting parameters from previous template_versions when updating a template #2397

Conversation

Emyrk commented Jun 16, 2022 • edited Loading

What this does

How it does this

Implementation

Alternatives tossed

Retrieve the param values and create the param requests

Imply inheriting parameters automatically

Having the cli pass some "inherit all params" indicator, rather than a CopyFromParameter for each needed param

Choose a reason for hiding this comment

johnstcn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnstcn Jun 16, 2022 • edited Loading

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

kylecarbs 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

Emyrk commented Jun 16, 2022 •

edited

Loading

Having the cli pass some "inherit all params" indicator, rather than a `CopyFromParameter` for each needed param

johnstcn Jun 16, 2022 •

edited

Loading