- Notifications
You must be signed in to change notification settings - Fork928
feat: Allow inheriting parameters from previous template_versions when updating a template#2397
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
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Made 1 query with filter options rather than adding new specific queries
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'm no expert on the params stuff, but I think this looks solid.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 making this opt-in
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yeah we're there now :D (args struct)
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 isusually the last working template version
Uh oh!
There was an error while loading.Please reload this page.
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
batch fetch params?
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.
Yea, I can make this change if this method is ok with the team
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.
Going to punt on this. We only have a handful of params right now. If it's a problem I can optimize later
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could we just inherit by default if not provided? Then we wouldn't needCopyFromParameter, and I believe we could delete a buncha 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.
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 theprovisioner 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.
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 haven't had time to run through the CLI experience, but this LGTM!
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What do you think aboutCopyFrom instead? OrCloneID? No specific reason, just preference.
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.
CloneID I like.
| @@ -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 | |||
| ifparameterFile != "" { | |||
| ifargs.ParameterFile != "" { | |||
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 we should remove theParameterFile 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The file logic needs to be looked at for sure. I kept it as is for now.
Uh oh!
There was an error while loading.Please reload this page.
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 noalways 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'dalways 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.