- Notifications
You must be signed in to change notification settings - Fork1.1k
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.
Changes from1 commit
d565cb5b6d3d67520fe49690daac692bab881ad901c917ab028b2b9f3fb8a222b642a6fd6cdf52b41b8f8a0dab1d9fc4ccfb2f08197a99aa9f3a8fdbc38bfaa7ab88c52dcd803eb22f4cb0f1452774c07File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
remove custom monotonic check
- Loading branch information
Uh oh!
There was an error while loading.Please reload this page.
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -543,6 +543,28 @@ func (s *server) acquireProtoJob(ctx context.Context, job database.ProvisionerJo | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, failJob(fmt.Sprintf("convert workspace transition: %s", err)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // A previous workspace build exists | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var lastWorkspaceBuildParameters []database.WorkspaceBuildParameter | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if workspaceBuild.BuildNumber > 1 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // TODO: Should we fetch the last build that succeeded? This fetches the | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // previous build regardless of the status of the build. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines +555 to +556 Member There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 🤷♂️ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| buildNum := workspaceBuild.BuildNumber - 1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| previous, err := s.Database.GetWorkspaceBuildByWorkspaceIDAndBuildNumber(ctx, database.GetWorkspaceBuildByWorkspaceIDAndBuildNumberParams{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| WorkspaceID: workspaceBuild.WorkspaceID, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| BuildNumber: buildNum, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines +555 to +561 Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. What problem are weactually trying to solve here?
coder/coderd/wsbuilder/wsbuilder.go Lines 650 to 669 inaf2941b
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? MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. That does it in The terraform provider now enforces monotonicity:coder/terraform-provider-coder#392 So this is duplicating that check in wsbuilder at MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I like that approach -- MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. @johnstcn actually, as much as I'd like to have 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
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 at Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Ahh... gotcha. That's unfortunate :( MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil && !xerrors.Is(err, sql.ErrNoRows) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, xerrors.Errorf("get last build with number=%d: %w", buildNum, err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err == nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| lastWorkspaceBuildParameters, err = s.Database.GetWorkspaceBuildParameters(ctx, previous.ID) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, xerrors.Errorf("get last build parameters %q: %w", previous.ID, err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| workspaceBuildParameters, err := s.Database.GetWorkspaceBuildParameters(ctx, workspaceBuild.ID) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return nil, failJob(fmt.Sprintf("get workspace build parameters: %s", err)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @@ -619,12 +641,13 @@ func (s *server) acquireProtoJob(ctx context.Context, job database.ProvisionerJo | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| protoJob.Type = &proto.AcquiredJob_WorkspaceBuild_{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| WorkspaceBuild: &proto.AcquiredJob_WorkspaceBuild{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| WorkspaceBuildId: workspaceBuild.ID.String(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| WorkspaceName: workspace.Name, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| State: workspaceBuild.ProvisionerState, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| RichParameterValues: convertRichParameterValues(workspaceBuildParameters), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| PreviousParameterValues: convertRichParameterValues(lastWorkspaceBuildParameters), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| VariableValues: asVariableValues(templateVariables), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ExternalAuthProviders: externalAuthProviders, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Metadata: &sdkproto.Metadata{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CoderUrl: s.AccessURL.String(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| WorkspaceTransition: transition, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.