Skip to content

feat: plumb Terraform resource address for script sync unit names #27154

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
SasSwart wants to merge 1 commit into
base: sas/script-units-on-timeline
Choose a base branch
from
Draft

feat: plumb Terraform resource address for script sync unit names #27154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions agent/agentscripts/agentscripts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,19 +148,20 @@ func (r *Runner) Init(scripts []codersdk.WorkspaceAgentScript, scriptCompleted S
// executes, giving observers a complete picture of pending work.
if r.UnitManager != nil {
for _, script := range scripts {
if script.DisplayName == "" {
r.Logger.Warn(r.cronCtx, "skipping unit registration for script with empty display name",
unitName := scriptUnitName(script)
if unitName == "" {
r.Logger.Warn(r.cronCtx, "skipping unit registration for script with no usable name",
slog.F("script_id", script.ID))
continue
}
if err := r.UnitManager.Register(unit.ID(script.DisplayName)); err != nil {
if err := r.UnitManager.Register(unit.ID(unitName)); err != nil {
if errors.Is(err, unit.ErrUnitAlreadyRegistered) {
r.Logger.Warn(r.cronCtx, "duplicate unit name during script registration",
slog.F("display_name", script.DisplayName),
slog.F("unit_name", unitName),
slog.F("script_id", script.ID))
continue
}
return xerrors.Errorf("register script unit %q: %w", script.DisplayName, err)
return xerrors.Errorf("register script unit %q: %w", unitName, err)
}
}
}
Expand Down Expand Up @@ -524,17 +525,28 @@ func (r *Runner) isClosed() bool {

// setUnitStatus updates the sync unit status for a script.
// It is a no-op when no UnitManager is configured or when the script
// has no DisplayName.
// has no usable unit name.
func (r *Runner) setUnitStatus(ctx context.Context, script codersdk.WorkspaceAgentScript, status unit.Status) {
if r.UnitManager == nil || script.DisplayName == "" {
unitName := scriptUnitName(script)
if r.UnitManager == nil || unitName == "" {
return
}
if err := r.UnitManager.UpdateStatus(unit.ID(script.DisplayName), status); err != nil {
if err := r.UnitManager.UpdateStatus(unit.ID(unitName), status); err != nil {
// Log but do not fail; unit tracking is best-effort relative
// to script execution.
r.Logger.Warn(ctx, "failed to update unit status",
slog.F("display_name", script.DisplayName),
slog.F("unit_name", unitName),
slog.F("status", string(status)),
slog.Error(err))
}
}

// scriptUnitName returns the unit name for a script. It prefers
// ResourceAddress (the Terraform resource address, unique across
// modules) and falls back to DisplayName.
func scriptUnitName(script codersdk.WorkspaceAgentScript) string {
if script.ResourceAddress != "" {
return script.ResourceAddress
}
return script.DisplayName
}
47 changes: 29 additions & 18 deletions agent/agentscripts/agentscripts_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,24 +389,33 @@ func TestScriptUnitsRegistered(t *testing.T) {

scripts := []codersdk.WorkspaceAgentScript{
{
ID: uuid.New(),
LogSourceID: uuid.New(),
DisplayName: "Install Tools",
Script: "echo install",
RunOnStart: true,
ID: uuid.New(),
LogSourceID: uuid.New(),
DisplayName: "Install Tools",
ResourceAddress: "coder_script.install_tools",
Script: "echo install",
RunOnStart: true,
},
{
ID: uuid.New(),
LogSourceID: uuid.New(),
DisplayName: "Configure Environment",
ResourceAddress: "module.dev.coder_script.configure",
Script: "echo configure",
RunOnStart: true,
},
{
ID: uuid.New(),
LogSourceID: uuid.New(),
DisplayName: "Configure Environment",
Script: "echo configure",
DisplayName: "", // No display name or resource address; should be skipped.
Script: "echo nameless",
RunOnStart: true,
},
{
ID: uuid.New(),
LogSourceID: uuid.New(),
DisplayName: "", // No display name; should be skipped.
Script: "echo nameless",
DisplayName: "Fallback Only", // No resource address; falls back to display name.
Script: "echo fallback",
RunOnStart: true,
},
}
Expand All @@ -415,16 +424,17 @@ func TestScriptUnitsRegistered(t *testing.T) {
err := runner.Init(scripts, aAPI.ScriptCompleted)
require.NoError(t, err)

// All scripts with a display name should be registered as pending.
// Three scripts should be registered (two with resource address, one with display name only).
units := mgr.ListUnits()
require.Len(t, units, 2)
require.Len(t, units, 3)

unitsByName := make(map[unit.ID]unit.Unit)
for _, u := range units {
unitsByName[u.ID()] = u
}

for _, name := range []string{"Install Tools", "Configure Environment"} {
// ResourceAddress takes precedence over DisplayName.
for _, name := range []string{"coder_script.install_tools", "module.dev.coder_script.configure", "Fallback Only"} {
u, ok := unitsByName[unit.ID(name)]
require.True(t, ok, "expected unit %q to be registered", name)
require.Equal(t, unit.StatusPending, u.Status(), "unit %q should be pending", name)
Expand All @@ -439,14 +449,15 @@ func TestScriptUnitsLifecycle(t *testing.T) {
})
defer runner.Close()

scriptName := "My Script"
scriptName := "coder_script.my_script"
scripts := []codersdk.WorkspaceAgentScript{
{
ID: uuid.New(),
LogSourceID: uuid.New(),
DisplayName: scriptName,
Script: "echo lifecycle",
RunOnStart: true,
ID: uuid.New(),
LogSourceID: uuid.New(),
DisplayName: "My Script",
ResourceAddress: scriptName,
Script: "echo lifecycle",
RunOnStart: true,
},
}

Expand Down
Loading
Loading