fix: Remove resource addresses (#982)

These were added under the impression that there was significant
user-experience impact if multiple resources share the same name.

This hasn't proven to be true yet, so figured we'd take this out
until it becomes necessary.
This commit is contained in:
Kyle Carberry
2022-04-12 14:38:02 -05:00
committed by GitHub
parent 52271ff9f8
commit e8b310166f
14 changed files with 17 additions and 171 deletions
+6 -5
View File
@@ -44,7 +44,7 @@ func WorkspaceResources(writer io.Writer, resources []codersdk.WorkspaceResource
if resource.Transition != database.WorkspaceTransitionStop { if resource.Transition != database.WorkspaceTransitionStop {
continue continue
} }
addressOnStop[resource.Address] = resource addressOnStop[resource.Type+"."+resource.Name] = resource
} }
// Displayed stores whether a resource has already been shown. // Displayed stores whether a resource has already been shown.
// Resources can be stored with numerous states, which we // Resources can be stored with numerous states, which we
@@ -75,24 +75,25 @@ func WorkspaceResources(writer io.Writer, resources []codersdk.WorkspaceResource
// callers to hide resources eventually. // callers to hide resources eventually.
continue continue
} }
if _, shown := displayed[resource.Address]; shown { resourceAddress := resource.Type + "." + resource.Name
if _, shown := displayed[resourceAddress]; shown {
// The same resource can have multiple transitions. // The same resource can have multiple transitions.
continue continue
} }
displayed[resource.Address] = struct{}{} displayed[resourceAddress] = struct{}{}
// Sort agents by name for consistent output. // Sort agents by name for consistent output.
sort.Slice(resource.Agents, func(i, j int) bool { sort.Slice(resource.Agents, func(i, j int) bool {
return resource.Agents[i].Name < resource.Agents[j].Name return resource.Agents[i].Name < resource.Agents[j].Name
}) })
_, existsOnStop := addressOnStop[resource.Address] _, existsOnStop := addressOnStop[resourceAddress]
resourceState := "ephemeral" resourceState := "ephemeral"
if existsOnStop { if existsOnStop {
resourceState = "persistent" resourceState = "persistent"
} }
// Display a line for the resource. // Display a line for the resource.
tableWriter.AppendRow(table.Row{ tableWriter.AppendRow(table.Row{
Styles.Bold.Render(resource.Type + "." + resource.Name), Styles.Bold.Render(resourceAddress),
Styles.Placeholder.Render(resourceState), Styles.Placeholder.Render(resourceState),
"", "",
}) })
-3
View File
@@ -42,17 +42,14 @@ func TestWorkspaceResources(t *testing.T) {
disconnected := database.Now().Add(-4 * time.Second) disconnected := database.Now().Add(-4 * time.Second)
go func() { go func() {
err := cliui.WorkspaceResources(ptty.Output(), []codersdk.WorkspaceResource{{ err := cliui.WorkspaceResources(ptty.Output(), []codersdk.WorkspaceResource{{
Address: "disk",
Transition: database.WorkspaceTransitionStart, Transition: database.WorkspaceTransitionStart,
Type: "google_compute_disk", Type: "google_compute_disk",
Name: "root", Name: "root",
}, { }, {
Address: "disk",
Transition: database.WorkspaceTransitionStop, Transition: database.WorkspaceTransitionStop,
Type: "google_compute_disk", Type: "google_compute_disk",
Name: "root", Name: "root",
}, { }, {
Address: "another",
Transition: database.WorkspaceTransitionStart, Transition: database.WorkspaceTransitionStart,
Type: "google_compute_instance", Type: "google_compute_instance",
Name: "dev", Name: "dev",
-3
View File
@@ -188,17 +188,14 @@ func main() {
RunE: func(cmd *cobra.Command, args []string) error { RunE: func(cmd *cobra.Command, args []string) error {
disconnected := database.Now().Add(-4 * time.Second) disconnected := database.Now().Add(-4 * time.Second)
return cliui.WorkspaceResources(cmd.OutOrStdout(), []codersdk.WorkspaceResource{{ return cliui.WorkspaceResources(cmd.OutOrStdout(), []codersdk.WorkspaceResource{{
Address: "disk",
Transition: database.WorkspaceTransitionStart, Transition: database.WorkspaceTransitionStart,
Type: "google_compute_disk", Type: "google_compute_disk",
Name: "root", Name: "root",
}, { }, {
Address: "disk",
Transition: database.WorkspaceTransitionStop, Transition: database.WorkspaceTransitionStop,
Type: "google_compute_disk", Type: "google_compute_disk",
Name: "root", Name: "root",
}, { }, {
Address: "another",
Transition: database.WorkspaceTransitionStart, Transition: database.WorkspaceTransitionStart,
Type: "google_compute_instance", Type: "google_compute_instance",
Name: "dev", Name: "dev",
@@ -1024,7 +1024,6 @@ func (q *fakeQuerier) InsertWorkspaceResource(_ context.Context, arg database.In
CreatedAt: arg.CreatedAt, CreatedAt: arg.CreatedAt,
JobID: arg.JobID, JobID: arg.JobID,
Transition: arg.Transition, Transition: arg.Transition,
Address: arg.Address,
Type: arg.Type, Type: arg.Type,
Name: arg.Name, Name: arg.Name,
} }
-1
View File
@@ -270,7 +270,6 @@ CREATE TABLE workspace_resources (
created_at timestamp with time zone NOT NULL, created_at timestamp with time zone NOT NULL,
job_id uuid NOT NULL, job_id uuid NOT NULL,
transition workspace_transition NOT NULL, transition workspace_transition NOT NULL,
address character varying(256) NOT NULL,
type character varying(192) NOT NULL, type character varying(192) NOT NULL,
name character varying(64) NOT NULL name character varying(64) NOT NULL
); );
@@ -66,7 +66,6 @@ CREATE TABLE workspace_resources (
created_at timestamptz NOT NULL, created_at timestamptz NOT NULL,
job_id uuid NOT NULL REFERENCES provisioner_jobs (id) ON DELETE CASCADE, job_id uuid NOT NULL REFERENCES provisioner_jobs (id) ON DELETE CASCADE,
transition workspace_transition NOT NULL, transition workspace_transition NOT NULL,
address varchar(256) NOT NULL,
type varchar(192) NOT NULL, type varchar(192) NOT NULL,
name varchar(64) NOT NULL, name varchar(64) NOT NULL,
PRIMARY KEY (id) PRIMARY KEY (id)
-1
View File
@@ -438,7 +438,6 @@ type WorkspaceResource struct {
CreatedAt time.Time `db:"created_at" json:"created_at"` CreatedAt time.Time `db:"created_at" json:"created_at"`
JobID uuid.UUID `db:"job_id" json:"job_id"` JobID uuid.UUID `db:"job_id" json:"job_id"`
Transition WorkspaceTransition `db:"transition" json:"transition"` Transition WorkspaceTransition `db:"transition" json:"transition"`
Address string `db:"address" json:"address"`
Type string `db:"type" json:"type"` Type string `db:"type" json:"type"`
Name string `db:"name" json:"name"` Name string `db:"name" json:"name"`
} }
+4 -17
View File
@@ -2518,7 +2518,7 @@ func (q *sqlQuerier) UpdateWorkspaceBuildByID(ctx context.Context, arg UpdateWor
const getWorkspaceResourceByID = `-- name: GetWorkspaceResourceByID :one const getWorkspaceResourceByID = `-- name: GetWorkspaceResourceByID :one
SELECT SELECT
id, created_at, job_id, transition, address, type, name id, created_at, job_id, transition, type, name
FROM FROM
workspace_resources workspace_resources
WHERE WHERE
@@ -2533,7 +2533,6 @@ func (q *sqlQuerier) GetWorkspaceResourceByID(ctx context.Context, id uuid.UUID)
&i.CreatedAt, &i.CreatedAt,
&i.JobID, &i.JobID,
&i.Transition, &i.Transition,
&i.Address,
&i.Type, &i.Type,
&i.Name, &i.Name,
) )
@@ -2542,7 +2541,7 @@ func (q *sqlQuerier) GetWorkspaceResourceByID(ctx context.Context, id uuid.UUID)
const getWorkspaceResourcesByJobID = `-- name: GetWorkspaceResourcesByJobID :many const getWorkspaceResourcesByJobID = `-- name: GetWorkspaceResourcesByJobID :many
SELECT SELECT
id, created_at, job_id, transition, address, type, name id, created_at, job_id, transition, type, name
FROM FROM
workspace_resources workspace_resources
WHERE WHERE
@@ -2563,7 +2562,6 @@ func (q *sqlQuerier) GetWorkspaceResourcesByJobID(ctx context.Context, jobID uui
&i.CreatedAt, &i.CreatedAt,
&i.JobID, &i.JobID,
&i.Transition, &i.Transition,
&i.Address,
&i.Type, &i.Type,
&i.Name, &i.Name,
); err != nil { ); err != nil {
@@ -2582,17 +2580,9 @@ func (q *sqlQuerier) GetWorkspaceResourcesByJobID(ctx context.Context, jobID uui
const insertWorkspaceResource = `-- name: InsertWorkspaceResource :one const insertWorkspaceResource = `-- name: InsertWorkspaceResource :one
INSERT INTO INSERT INTO
workspace_resources ( workspace_resources (id, created_at, job_id, transition, type, name)
id,
created_at,
job_id,
transition,
address,
type,
name
)
VALUES VALUES
($1, $2, $3, $4, $5, $6, $7) RETURNING id, created_at, job_id, transition, address, type, name ($1, $2, $3, $4, $5, $6) RETURNING id, created_at, job_id, transition, type, name
` `
type InsertWorkspaceResourceParams struct { type InsertWorkspaceResourceParams struct {
@@ -2600,7 +2590,6 @@ type InsertWorkspaceResourceParams struct {
CreatedAt time.Time `db:"created_at" json:"created_at"` CreatedAt time.Time `db:"created_at" json:"created_at"`
JobID uuid.UUID `db:"job_id" json:"job_id"` JobID uuid.UUID `db:"job_id" json:"job_id"`
Transition WorkspaceTransition `db:"transition" json:"transition"` Transition WorkspaceTransition `db:"transition" json:"transition"`
Address string `db:"address" json:"address"`
Type string `db:"type" json:"type"` Type string `db:"type" json:"type"`
Name string `db:"name" json:"name"` Name string `db:"name" json:"name"`
} }
@@ -2611,7 +2600,6 @@ func (q *sqlQuerier) InsertWorkspaceResource(ctx context.Context, arg InsertWork
arg.CreatedAt, arg.CreatedAt,
arg.JobID, arg.JobID,
arg.Transition, arg.Transition,
arg.Address,
arg.Type, arg.Type,
arg.Name, arg.Name,
) )
@@ -2621,7 +2609,6 @@ func (q *sqlQuerier) InsertWorkspaceResource(ctx context.Context, arg InsertWork
&i.CreatedAt, &i.CreatedAt,
&i.JobID, &i.JobID,
&i.Transition, &i.Transition,
&i.Address,
&i.Type, &i.Type,
&i.Name, &i.Name,
) )
+2 -10
View File
@@ -16,14 +16,6 @@ WHERE
-- name: InsertWorkspaceResource :one -- name: InsertWorkspaceResource :one
INSERT INTO INSERT INTO
workspace_resources ( workspace_resources (id, created_at, job_id, transition, type, name)
id,
created_at,
job_id,
transition,
address,
type,
name
)
VALUES VALUES
($1, $2, $3, $4, $5, $6, $7) RETURNING *; ($1, $2, $3, $4, $5, $6) RETURNING *;
+5 -15
View File
@@ -27,7 +27,6 @@ import (
"github.com/coder/coder/coderd/httpapi" "github.com/coder/coder/coderd/httpapi"
"github.com/coder/coder/coderd/parameter" "github.com/coder/coder/coderd/parameter"
"github.com/coder/coder/provisionerd/proto" "github.com/coder/coder/provisionerd/proto"
"github.com/coder/coder/provisionersdk"
sdkproto "github.com/coder/coder/provisionersdk/proto" sdkproto "github.com/coder/coder/provisionersdk/proto"
) )
@@ -475,18 +474,14 @@ func (server *provisionerdServer) CompleteJob(ctx context.Context, completed *pr
database.WorkspaceTransitionStart: jobType.TemplateImport.StartResources, database.WorkspaceTransitionStart: jobType.TemplateImport.StartResources,
database.WorkspaceTransitionStop: jobType.TemplateImport.StopResources, database.WorkspaceTransitionStop: jobType.TemplateImport.StopResources,
} { } {
addresses, err := provisionersdk.ResourceAddresses(resources) for _, resource := range resources {
if err != nil {
return nil, xerrors.Errorf("compute resource addresses: %w", err)
}
for index, resource := range resources {
server.Logger.Info(ctx, "inserting template import job resource", server.Logger.Info(ctx, "inserting template import job resource",
slog.F("job_id", job.ID.String()), slog.F("job_id", job.ID.String()),
slog.F("resource_name", resource.Name), slog.F("resource_name", resource.Name),
slog.F("resource_type", resource.Type), slog.F("resource_type", resource.Type),
slog.F("transition", transition)) slog.F("transition", transition))
err = insertWorkspaceResource(ctx, server.Database, jobID, transition, resource, addresses[index]) err = insertWorkspaceResource(ctx, server.Database, jobID, transition, resource)
if err != nil { if err != nil {
return nil, xerrors.Errorf("insert resource: %w", err) return nil, xerrors.Errorf("insert resource: %w", err)
} }
@@ -540,13 +535,9 @@ func (server *provisionerdServer) CompleteJob(ctx context.Context, completed *pr
if err != nil { if err != nil {
return xerrors.Errorf("update workspace build: %w", err) return xerrors.Errorf("update workspace build: %w", err)
} }
addresses, err := provisionersdk.ResourceAddresses(jobType.WorkspaceBuild.Resources)
if err != nil {
return xerrors.Errorf("compute resource addresses: %w", err)
}
// This could be a bulk insert to improve performance. // This could be a bulk insert to improve performance.
for index, protoResource := range jobType.WorkspaceBuild.Resources { for _, protoResource := range jobType.WorkspaceBuild.Resources {
err = insertWorkspaceResource(ctx, db, job.ID, workspaceBuild.Transition, protoResource, addresses[index]) err = insertWorkspaceResource(ctx, db, job.ID, workspaceBuild.Transition, protoResource)
if err != nil { if err != nil {
return xerrors.Errorf("insert provisioner job: %w", err) return xerrors.Errorf("insert provisioner job: %w", err)
} }
@@ -578,13 +569,12 @@ func (server *provisionerdServer) CompleteJob(ctx context.Context, completed *pr
return &proto.Empty{}, nil return &proto.Empty{}, nil
} }
func insertWorkspaceResource(ctx context.Context, db database.Store, jobID uuid.UUID, transition database.WorkspaceTransition, protoResource *sdkproto.Resource, address string) error { func insertWorkspaceResource(ctx context.Context, db database.Store, jobID uuid.UUID, transition database.WorkspaceTransition, protoResource *sdkproto.Resource) error {
resource, err := db.InsertWorkspaceResource(ctx, database.InsertWorkspaceResourceParams{ resource, err := db.InsertWorkspaceResource(ctx, database.InsertWorkspaceResourceParams{
ID: uuid.New(), ID: uuid.New(),
CreatedAt: database.Now(), CreatedAt: database.Now(),
JobID: jobID, JobID: jobID,
Transition: transition, Transition: transition,
Address: address,
Type: protoResource.Type, Type: protoResource.Type,
Name: protoResource.Name, Name: protoResource.Name,
}) })
-1
View File
@@ -110,7 +110,6 @@ func convertWorkspaceResource(resource database.WorkspaceResource, agents []code
CreatedAt: resource.CreatedAt, CreatedAt: resource.CreatedAt,
JobID: resource.JobID, JobID: resource.JobID,
Transition: resource.Transition, Transition: resource.Transition,
Address: resource.Address,
Type: resource.Type, Type: resource.Type,
Name: resource.Name, Name: resource.Name,
Agents: agents, Agents: agents,
-1
View File
@@ -25,7 +25,6 @@ type WorkspaceResource struct {
CreatedAt time.Time `json:"created_at"` CreatedAt time.Time `json:"created_at"`
JobID uuid.UUID `json:"job_id"` JobID uuid.UUID `json:"job_id"`
Transition database.WorkspaceTransition `json:"workspace_transition"` Transition database.WorkspaceTransition `json:"workspace_transition"`
Address string `json:"address"`
Type string `json:"type"` Type string `json:"type"`
Name string `json:"name"` Name string `json:"name"`
Agents []WorkspaceAgent `json:"agents,omitempty"` Agents []WorkspaceAgent `json:"agents,omitempty"`
-49
View File
@@ -1,49 +0,0 @@
package provisionersdk
import (
"fmt"
"golang.org/x/xerrors"
"github.com/coder/coder/provisionersdk/proto"
)
// ResourceAddresses returns an index-matching slice of unique addresses
// to access resources.
func ResourceAddresses(resources []*proto.Resource) ([]string, error) {
resourcesByHost := map[string]*proto.Resource{}
for _, resource := range resources {
otherByName, exists := resourcesByHost[resource.Name]
if !exists {
resourcesByHost[resource.Name] = resource
continue
}
// If we have conflicting names, to reduce confusion we prepend the types.
delete(resourcesByHost, otherByName.Name)
otherAddress := fmt.Sprintf("%s.%s", otherByName.Type, otherByName.Name)
resourcesByHost[otherAddress] = otherByName
address := fmt.Sprintf("%s.%s", resource.Type, resource.Name)
_, exists = resourcesByHost[address]
if !exists {
resourcesByHost[address] = resource
continue
}
return nil, xerrors.Errorf("found resource with conflicting address %q", otherAddress)
}
addresses := make([]string, 0, len(resources))
for _, resource := range resources {
found := false
for host, other := range resourcesByHost {
if resource != other {
continue
}
found = true
addresses = append(addresses, host)
}
if !found {
panic(fmt.Sprintf("dev error: resource %s.%s wasn't given an address", resource.Type, resource.Name))
}
}
return addresses, nil
}
-63
View File
@@ -1,63 +0,0 @@
package provisionersdk_test
import (
"testing"
"github.com/stretchr/testify/require"
"github.com/coder/coder/provisionersdk"
"github.com/coder/coder/provisionersdk/proto"
)
func TestResourceAddresses(t *testing.T) {
t.Parallel()
t.Run("Single", func(t *testing.T) {
t.Parallel()
addresses, err := provisionersdk.ResourceAddresses([]*proto.Resource{{
Type: "google_compute_instance",
Name: "dev",
}})
require.NoError(t, err)
require.Len(t, addresses, 1)
require.Equal(t, addresses[0], "dev")
})
t.Run("Multiple", func(t *testing.T) {
t.Parallel()
addresses, err := provisionersdk.ResourceAddresses([]*proto.Resource{{
Type: "google_compute_instance",
Name: "linux",
}, {
Type: "google_compute_instance",
Name: "windows",
}})
require.NoError(t, err)
require.Len(t, addresses, 2)
require.Equal(t, addresses[0], "linux")
require.Equal(t, addresses[1], "windows")
})
t.Run("ConflictingDifferent", func(t *testing.T) {
t.Parallel()
addresses, err := provisionersdk.ResourceAddresses([]*proto.Resource{{
Type: "google_compute_instance",
Name: "dev",
}, {
Type: "kubernetes_pod",
Name: "dev",
}})
require.NoError(t, err)
require.Len(t, addresses, 2)
require.Equal(t, addresses[0], "google_compute_instance.dev")
require.Equal(t, addresses[1], "kubernetes_pod.dev")
})
t.Run("ConflictingSame", func(t *testing.T) {
t.Parallel()
_, err := provisionersdk.ResourceAddresses([]*proto.Resource{{
Type: "google_compute_instance",
Name: "dev",
}, {
Type: "google_compute_instance",
Name: "dev",
}})
require.Error(t, err)
})
}