Do we need to be worried about a builder being used across multiple goroutines?

Depends on what you mean.

When you are configuring the builder, it's passed around by value, so it's safe to use in multiple go routines. However, it's not safe to call b.Build() from more than one goroutine. It's hard to imagine why you would do that, as we explicitly ensure that only one build can be running at a time per workspace.

If, however, you're talking about multiple goroutines attempting to insert a build on the same logical workspace (i.e. the goroutines don't share memory, but they are operating on the same database object), then the use of database transactions will ensure at most one build is inserted.

👍

Given that this new package is taking on the burden of being the one-stop-shop for building workspaces, it might be a good idea to add dedicated unit tests for the package instead of relying on the consuming packages to do the job for us.

nit: Wasn't that intentionally skipped not to leak any secret values?

maybe?

I don't consider it a security risk to allow users to read back out the values that they themselves created. It's very useful, and we allow it in the case of Rich Parameters.

In this case, we use it while migrating legacy parameters to rich parameters in codersdk.ParameterResolver.

I see your point, maybe it is not so bad.

In the case of rich parameters (vel. workspace build parameters), it is true. Template owners can use managed template variables to hide sensitive information (property sensitive).

nit: should we add some context to all return nil, err for debugging purposes?

nit: should we unexpose these properties or do you intend to use them in future PRs?

They're exposed so you can do

resolver := &codersdk.ParameterResolver{
    Legacy: legacyParams,
    Rich: richParams,
}

Otherwise we'd need a NewParameterResolver() function to initialize this struct from outside the package.

Ok, so the only questionable thing is the location of ParameterResolver, but I think that I have already raised this concern in some other comment.

I know that we're testing this logic mostly in the coderd API tests, but maybe a few extra unit tests here will be useful.

maybe add some error wrapping here if this error is really unexpected.

Can we expose these on the struct directly and remove these helper functions?

Then the New func could be discarded too if we wanted, which could be nice.

We can.

I'll admit I do like the Fluent interface that we'd lose.

builder := wsbuilder.New(workspace, database.WorkspaceTransitionStart).
    Reason(database.BuildReasonInitiator).
    Initiator(apiKey.UserID).
    ActiveVersion().
    LegacyParameterValues(createWorkspace.ParameterValues).
    RichParameterValues(createWorkspace.RichParameterValues).
    State(state)

Would become

builder := wsbuilder.Builder{
    Workspace: workspace,
    Transition: database.WorkspaceTransitionStart,
    Reason: database.BuildReasonInitiator,
    Initiator: apiKey.UserID,
    Version: VersionTarget{Active: true},
    LegacyParameterValues: createWorkspace.ParameterValues,
    RichParameterValues: createWorkspace.RichParameterValues,
    State: StateTarget{Explicit: state}
}

I'm playing around with the ...opts pattern that we use elsewhere in the codebase, but I don't like it for this.

	builder := wsbuilder.New(
		workspace, database.WorkspaceTransition(createBuild.Transition),
		wsbuilder.Initiator(apiKey.UserID),
		wsbuilder.LegacyParameterValues(createBuild.ParameterValues),
		wsbuilder.RichParameterValues(createBuild.RichParameterValues),
		wsbuilder.LogLevel(string(createBuild.LogLevel)),
	)

	if createBuild.TemplateVersionID != uuid.Nil {
		builder = wsbuilder.VersionID(createBuild.TemplateVersionID)(builder)
	}

You have to type wsbuilder. a bunch of extra times, and it's more awkward to modify the builder after the first call to New(), as in the VersionID call above.

Seems like all of these can happen in parallel and use an errgroup.

That adds a fair amount of complexity.

The method calls read & modify an internal cache of db objects, which would have to have a mutex protecting it if it gets accessed from multiple goroutines. Holding that lock while querying the database would bring us back serial requests, but not holding the lock opens a big can of worms where you have to track requests in flight, lest we query the database multiple times for the same thing. So, the database calls get partially serialized as a natural consequence, meaning the level of parallelism we can achieve isn't that great.

I say not worth it!

This can also use an errgroup

Can we make that a later optimization? Like if we have some traces or whatever that show this is a bottleneck.

I feel like your typical template has like 5 or fewer parameters, so serializing the calls isn't a big deal. It's what the code I'm refactoring from does.

There's arguably some redundancy because of the name db inside the database package, but it would be kinda nice if this was called dbmock instead for consistency.