feat: Allow inheriting parameters from previous template_versions when updating a template #2397
Conversation
- Allow more columns on template list
| @@ -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) { | |||
There was a problem hiding this comment.
Made 1 query with filter options rather than adding new specific queries
cli/templatecreate.go
Outdated
| // 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 |
There was a problem hiding this comment.
I plan to add some kind of --always-prompt flag if you want to change the params.
| ReuseParams bool | ||
| } | ||
|
|
||
| func createValidTemplateVersion(cmd *cobra.Command, args createValidTemplateVersionArgs, parameters ...codersdk.CreateParameterRequest) (*codersdk.TemplateVersion, []codersdk.CreateParameterRequest, error) { |
There was a problem hiding this comment.
yeah we're there now :D (args struct)
There was a problem hiding this comment.
I really want to try refactoring this function. It's growing limbs and becoming quite large lol.
"Get it working" was my first step...
cli/templatecreate.go
Outdated
| // 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 | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| // This is a bit inefficient, as we make a new db call for each | ||
| // param. | ||
| version, err := db.GetTemplateVersionByJobID(r.Context(), copy.ScopeID) |
There was a problem hiding this comment.
Yea, I can make this change if this method is ok with the team
There was a problem hiding this comment.
Going to punt on this. We only have a handful of params right now. If it's a problem I can optimize later
coderd/templateversions.go
Outdated
| inherits := make([]uuid.UUID, 0) | ||
| for _, parameterValue := range req.ParameterValues { | ||
| if parameterValue.CopyFromParameter != uuid.Nil { | ||
| inherits = append(inherits, parameterValue.CopyFromParameter) | ||
| } | ||
| } |
There was a problem hiding this comment.
Could we just inherit by default if not provided? Then we wouldn't need CopyFromParameter, and I believe we could delete a buncha this!
There was a problem hiding this comment.
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.
codersdk/parameters.go
Outdated
| // 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"` |
There was a problem hiding this comment.
What do you think about CopyFrom instead? Or CloneID? No specific reason, just preference.
| @@ -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 != "" { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The file logic needs to be looked at for sure. I kept it as is for now.
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:
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
CreateParameterRequestcalledCopyFromParameterwhich is a uuid that points to another parameter. A list ofCreateParameterRequests are sent to the backend with the template_version create.The CLI then when finding it is missing parameters:
import_jobscope (and only that scope)CopyFromParameterfor the missing params.The backend then can fetch the parameter from
CopyFromParameterand check a few things:template_id(cannot import from other templtes)import_jobAlternatives 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
CopyFromParameterfor each needed paramBecause 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.