- Notifications
You must be signed in to change notification settings - Fork911
chore: pass previous values into terraform apply#17696
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
remove custom monotonic check
| // TODO: Should we fetch the last build that succeeded? This fetches the | ||
| // previous build regardless of the status of the build. | ||
| buildNum := workspaceBuild.BuildNumber - 1 | ||
| previous, err := s.Database.GetWorkspaceBuildByWorkspaceIDAndBuildNumber(ctx, database.GetWorkspaceBuildByWorkspaceIDAndBuildNumberParams{ | ||
| WorkspaceID: workspaceBuild.WorkspaceID, | ||
| BuildNumber: buildNum, | ||
| }) |
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 problem are weactually trying to solve here?
wsbuilder already fetches the last build parameters, if they exist:
coder/coderd/wsbuilder/wsbuilder.go
Lines 650 to 669 inaf2941b
| func (b*Builder)getLastBuildParameters() ([]database.WorkspaceBuildParameter,error) { | |
| ifb.lastBuildParameters!=nil { | |
| return*b.lastBuildParameters,nil | |
| } | |
| bld,err:=b.getLastBuild() | |
| ifxerrors.Is(err,sql.ErrNoRows) { | |
| // if the build doesn't exist, then clearly there can be no parameters. | |
| b.lastBuildParameters=&[]database.WorkspaceBuildParameter{} | |
| return*b.lastBuildParameters,nil | |
| } | |
| iferr!=nil { | |
| returnnil,xerrors.Errorf("get last build to get parameters: %w",err) | |
| } | |
| values,err:=b.store.GetWorkspaceBuildParameters(b.ctx,bld.ID) | |
| iferr!=nil&&!xerrors.Is(err,sql.ErrNoRows) { | |
| returnnil,xerrors.Errorf("get last build %s parameters: %w",bld.ID,err) | |
| } | |
| b.lastBuildParameters=&values | |
| returnvalues,nil | |
| } |
Given that this is the case, why do we need to do this extra work forall jobs? Isn't this just for template version import jobs?
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.
That does it incoder/coder at workspace create yes, but this passes the previous values to the terraform via env vars.
The terraform provider now enforces monotonicity:coder/terraform-provider-coder#392
So this is duplicating that check in wsbuilder atterraform apply/plan.
For dynamic parameters, we skip validating params inwsbuilder, so we need to make sure validation is applied in terraform
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.
Maybe I could pass the values from wsbuilder to here via the job? Rather than refetch
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 like that approach --wsbuilder is then still responsible for fetching all of the various baggage related to a workspace build, but just defers the validation part to Terraform.
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.
@johnstcn actually, as much as I'd like to havewsbuilder be the source of truth. We fetch everything again at this step in the workspace build.
Current params, workspace data, external auth, etc.
We store very little in the job payload:
coder/coderd/provisionerdserver/provisionerdserver.go
Lines 2498 to 2504 ina7ab88c
| typeWorkspaceProvisionJobstruct { | |
| WorkspaceBuildID uuid.UUID`json:"workspace_build_id"` | |
| DryRunbool`json:"dry_run"` | |
| LogLevelstring`json:"log_level,omitempty"` | |
| PrebuiltWorkspaceBuildStage sdkproto.PrebuiltWorkspaceBuildStage`json:"prebuilt_workspace_stage,omitempty"` | |
| } | |
So I'm going to keep this as a refetching. Ideally I would use the same function to fetch the previous params in both cases, however atwsbuild thelatestbuild is the "previous". And at the point I added code, the previous isbuild -1.
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.
Ahh... gotcha. That's unfortunate :(
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.
It is, I think it would be a large refactor to move all the fields into wsbuilder
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 need to updateprovisionerd/proto/version.go when changes to this file are made.
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 did this. There is another change that bumped to v1.5 today. So I'm joining that minor version bump.
| // TODO: Should we fetch the last build that succeeded? This fetches the | ||
| // previous build regardless of the status of the build. |
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.
If we check for the last successful build, we could end up with no builds. What do we do then? Do we just settle for the last build? IMO just checking the previous build is simpler conceptually, and is more likely to be what users expect.
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, the wsbuilder just takes the last build regardless of status. Just feels a bit off since the tfstate is different if the previous failed. 🤷♂️
| // - Add previous parameter values to 'WorkspaceBuild' jobs. Provisioner passes | ||
| // the previous values for the `terraform apply` to enforce monotonicity | ||
| // in the terraform provider. |
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.
👍
398b999 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Pass previous workspace build parameter values into the terraform
plan/apply. Enforces monotonicity in terraform as well ascoderd.