Skip to content

fix: nctl silently override service with same name #449

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

Merged
SaadAssaf merged 1 commit into from
Sep 7, 2026
Merged

fix: nctl silently override service with same name #449

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
44 changes: 22 additions & 22 deletions create/application.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,27 +38,27 @@ const DefaultReplicas = 2
// update/application.go.
type applicationCmd struct {
ResourceCmd
Git gitConfig `embed:"" prefix:"git-"`
Size *string `help:"Size of the application (defaults to \"${app_default_size}\")." placeholder:"${app_default_size}"`
Port *int32 `help:"Port the application is listening on (defaults to ${app_default_port})." placeholder:"${app_default_port}"`
HealthProbe healthProbe `embed:"" prefix:"health-probe-"`
Replicas int32 `help:"Amount of replicas of the running application (defaults to ${app_default_replicas})." placeholder:"${app_default_replicas}" default:"${app_default_replicas}"`
Hosts []string `help:"Host names where the application can be accessed. If empty, the application will just be accessible on a generated host name on the deploio.app domain."`
BasicAuth *bool `help:"Enable/Disable basic authentication for the application (defaults to ${app_default_basic_auth})." placeholder:"${app_default_basic_auth}"`
Env map[string]string `help:"Environment variables which are passed to the application at runtime."`
SensitiveEnv map[string]string `help:"Sensitive environment variables which are passed to the application at runtime."`
BuildEnv map[string]string `help:"Environment variables which are passed to the application build process."`
SensitiveBuildEnv map[string]string `help:"Sensitive environment variables which are passed to the application build process."`
Service application.ServiceMap `help:"Service reference in the form name=kind/target-name. Credentials will be automatically injected as environment variables."`
DeployJob deployJob `embed:"" prefix:"deploy-job-"`
WorkerJob workerJob `embed:"" prefix:"worker-job-"`
ScheduledJob scheduledJob `embed:"" prefix:"scheduled-job-"`
GitInformationServiceURL string `help:"URL of the git information service." default:"https://git-info.deplo.io" env:"GIT_INFORMATION_SERVICE_URL" hidden:""`
SkipRepoAccessCheck bool `help:"Skip the git repository access check." default:"false"`
Debug bool `help:"Enable debug messages." default:"false"`
Language string `help:"${app_language_help} Possible values: ${enum}" enum:"ruby,php,python,golang,nodejs,static," default:""`
DockerfileBuild dockerfileBuild `embed:""`
BuildpackStack string `help:"${app_buildpack_stack_help} Possible values: ${enum}" enum:"paketo,heroku," default:""`
Git gitConfig `embed:"" prefix:"git-"`
Size *string `help:"Size of the application (defaults to \"${app_default_size}\")." placeholder:"${app_default_size}"`
Port *int32 `help:"Port the application is listening on (defaults to ${app_default_port})." placeholder:"${app_default_port}"`
HealthProbe healthProbe `embed:"" prefix:"health-probe-"`
Replicas int32 `help:"Amount of replicas of the running application (defaults to ${app_default_replicas})." placeholder:"${app_default_replicas}" default:"${app_default_replicas}"`
Hosts []string `help:"Host names where the application can be accessed. If empty, the application will just be accessible on a generated host name on the deploio.app domain."`
BasicAuth *bool `help:"Enable/Disable basic authentication for the application (defaults to ${app_default_basic_auth})." placeholder:"${app_default_basic_auth}"`
Env map[string]string `help:"Environment variables which are passed to the application at runtime."`
SensitiveEnv map[string]string `help:"Sensitive environment variables which are passed to the application at runtime."`
BuildEnv map[string]string `help:"Environment variables which are passed to the application build process."`
SensitiveBuildEnv map[string]string `help:"Sensitive environment variables which are passed to the application build process."`
Service []application.NamedServiceReference `sep:"none" help:"Service reference in the form name=kind/target-name. Credentials will be automatically injected as environment variables. Repeat the flag to pass more than one service."`
DeployJob deployJob `embed:"" prefix:"deploy-job-"`
WorkerJob workerJob `embed:"" prefix:"worker-job-"`
ScheduledJob scheduledJob `embed:"" prefix:"scheduled-job-"`
GitInformationServiceURL string `help:"URL of the git information service." default:"https://git-info.deplo.io" env:"GIT_INFORMATION_SERVICE_URL" hidden:""`
SkipRepoAccessCheck bool `help:"Skip the git repository access check." default:"false"`
Debug bool `help:"Enable debug messages." default:"false"`
Language string `help:"${app_language_help} Possible values: ${enum}" enum:"ruby,php,python,golang,nodejs,static," default:""`
DockerfileBuild dockerfileBuild `embed:""`
BuildpackStack string `help:"${app_buildpack_stack_help} Possible values: ${enum}" enum:"paketo,heroku," default:""`
}

type gitConfig struct {
Expand Down Expand Up @@ -401,7 +401,7 @@ func (cmd *applicationCmd) newApplication(project string) *apps.Application {
Hosts: cmd.Hosts,
Config: cmd.config(),
BuildEnv: combineEnvVars(cmd.BuildEnv, cmd.SensitiveBuildEnv),
Services: application.ServicesFromMap(cmd.Service, project),
Services: application.ServicesFromReferences(cmd.Service, project),
DockerfileBuild: apps.DockerfileBuild{
Enabled: cmd.DockerfileBuild.Enabled,
DockerfilePath: cmd.DockerfileBuild.Path,
Expand Down
10 changes: 5 additions & 5 deletions create/application_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,15 +571,15 @@ func TestCreateApplication(t *testing.T) {
URL: "https://github.com/ninech/doesnotexist.git",
Revision: "main",
},
Service: func() application.ServiceMap {
m := application.ServiceMap{}
Service: func() []application.NamedServiceReference {
cache := application.TypedReference{}
cache.UnmarshalText([]byte("keyvaluestore/my-kvs"))
m["cache"] = cache
db := application.TypedReference{}
db.UnmarshalText([]byte("mysql/my-db"))
m["db"] = db
return m
return []application.NamedServiceReference{
{Name: "cache", Target: cache},
{Name: "db", Target: db},
}
}(),
SkipRepoAccessCheck: true,
},
Expand Down
36 changes: 27 additions & 9 deletions internal/application/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,28 +2,46 @@ package application

import (
"cmp"
"fmt"
"slices"
"strings"

apps "github.com/ninech/apis/apps/v1alpha1"
"github.com/ninech/nctl/internal/format"
)

// ServiceMap is a map of service name to typed reference, used as a CLI flag type.
type ServiceMap map[string]TypedReference
// NamedServiceReference is a named reference to a service target, in the
// form "name=kind/target-name".
type NamedServiceReference struct {
Name string
Target TypedReference
}

// UnmarshalText parses a named service reference from a string in
// "name=kind/target-name" format.
func (n *NamedServiceReference) UnmarshalText(text []byte) error {
name, rest, found := strings.Cut(string(text), "=")
if !found || name == "" {
return fmt.Errorf("unmarshal error: expected name=kind/target, got %q", text)
}
n.Name = name
return n.Target.UnmarshalText([]byte(rest))
}

// ServicesFromMap converts a map of name -> TypedReference into a
// ServicesFromReferences converts a slice of NamedServiceReference into a
// NamedServiceTargetList. The namespace is set on each target.
func ServicesFromMap(services ServiceMap, namespace string) apps.NamedServiceTargetList {
func ServicesFromReferences(services []NamedServiceReference, namespace string) apps.NamedServiceTargetList {
if len(services) == 0 {
return nil
}

result := make(apps.NamedServiceTargetList, 0, len(services))
for name, ref := range services {
ref.Namespace = namespace
for _, ref := range services {
target := ref.Target.TypedReference
target.Namespace = namespace
result = append(result, apps.NamedServiceTarget{
Name: name,
Target: ref.TypedReference,
Name: ref.Name,
Target: target,
})
}

Expand All @@ -41,7 +59,7 @@ func UpdateServices(existing apps.NamedServiceTargetList, toAdd apps.NamedServic
for _, add := range toAdd {
found := false
for i := range existing {
if existing[i].Name == add.Name {
if existing[i].Name == add.Name && existing[i].Target.Kind == add.Target.Kind {
existing[i].Target = add.Target
found = true
break
Expand Down
57 changes: 49 additions & 8 deletions internal/application/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

func TestServicesFromMap(t *testing.T) {
func TestServicesFromReferences(t *testing.T) {
t.Parallel()

kvsRef := TypedReference{}
Expand All @@ -23,18 +23,21 @@ func TestServicesFromMap(t *testing.T) {

tests := []struct {
name string
services ServiceMap
services []NamedServiceReference
namespace string
want apps.NamedServiceTargetList
// unordered is needed for entries sharing a name, since sort order
// among them is not guaranteed.
unordered bool
}{
{
name: "nil map",
name: "nil slice",
services: nil,
want: nil,
},
{
name: "single service",
services: ServiceMap{"cache": kvsRef},
services: []NamedServiceReference{{Name: "cache", Target: kvsRef}},
namespace: "my-project",
want: apps.NamedServiceTargetList{
{
Expand All @@ -48,9 +51,9 @@ func TestServicesFromMap(t *testing.T) {
},
{
name: "multiple services sorted",
services: ServiceMap{
"db": mysqlRef,
"cache": kvsRef,
services: []NamedServiceReference{
{Name: "db", Target: mysqlRef},
{Name: "cache", Target: kvsRef},
},
namespace: "default",
want: apps.NamedServiceTargetList{
Expand All @@ -70,11 +73,40 @@ func TestServicesFromMap(t *testing.T) {
},
},
},
{
name: "same name different kind both preserved",
services: []NamedServiceReference{
{Name: "billy", Target: mysqlRef},
{Name: "billy", Target: kvsRef},
},
namespace: "default",
want: apps.NamedServiceTargetList{
{
Name: "billy",
Target: meta.TypedReference{
Reference: meta.Reference{Name: "my-db", Namespace: "default"},
GroupKind: mysqlRef.GroupKind,
},
},
{
Name: "billy",
Target: meta.TypedReference{
Reference: meta.Reference{Name: "my-kvs", Namespace: "default"},
GroupKind: kvsRef.GroupKind,
},
},
},
unordered: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
got := ServicesFromMap(tt.services, tt.namespace)
got := ServicesFromReferences(tt.services, tt.namespace)
if tt.unordered {
require.ElementsMatch(t, tt.want, got)
return
}
require.Equal(t, tt.want, got)
})
}
Expand Down Expand Up @@ -176,6 +208,15 @@ func TestUpdateServices(t *testing.T) {
toDelete: []string{"cache"},
want: apps.NamedServiceTargetList{{Name: "db", Target: mysqlTarget}},
},
{
name: "same name different kind",
existing: apps.NamedServiceTargetList{{Name: "cache", Target: kvsTarget}},
toAdd: apps.NamedServiceTargetList{{Name: "cache", Target: mysqlTarget}},
want: apps.NamedServiceTargetList{
{Name: "cache", Target: kvsTarget},
{Name: "cache", Target: mysqlTarget},
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down
34 changes: 17 additions & 17 deletions update/application.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,22 +49,22 @@ type applicationCmd struct {
// structs. Due to the usage of kong these pointers will never be `nil`.
// So checking for `nil` values can not be used to find out if some of
// the struct fields have been set.
DeployJob *deployJob `embed:"" prefix:"deploy-job-"`
WorkerJob *workerJob `embed:"" prefix:"worker-job-"`
ScheduledJob *scheduledJob `embed:"" prefix:"scheduled-job-"`
DeleteWorkerJob *string `help:"Delete a worker job by name."`
DeleteScheduledJob *string `help:"Delete a scheduled job by name."`
Service application.ServiceMap `help:"Service reference to add/update in the form name=kind/target-name."`
DeleteService []string `help:"Service reference names to remove."`
RetryRelease *bool `help:"Retries release for the application." placeholder:"false"`
RetryBuild *bool `help:"Retries build for the application if set to true." placeholder:"false"`
Pause *bool `negatable:"" help:"Pause or unpause the application. Pausing stops all costs."`
GitInformationServiceURL string `help:"URL of the git information service." default:"https://git-info.deplo.io" env:"GIT_INFORMATION_SERVICE_URL" hidden:""`
SkipRepoAccessCheck bool `help:"Skip the git repository access check." default:"false"`
Debug bool `help:"Enable debug messages." default:"false"`
Language *string `help:"${app_language_help} Possible values: ${enum}" enum:"ruby,php,python,golang,nodejs,static,"`
DockerfileBuild dockerfileBuild `embed:""`
BuildpackStack *string `help:"${app_buildpack_stack_help} Possible values: ${enum}" enum:"paketo,heroku,"`
DeployJob *deployJob `embed:"" prefix:"deploy-job-"`
WorkerJob *workerJob `embed:"" prefix:"worker-job-"`
ScheduledJob *scheduledJob `embed:"" prefix:"scheduled-job-"`
DeleteWorkerJob *string `help:"Delete a worker job by name."`
DeleteScheduledJob *string `help:"Delete a scheduled job by name."`
Service []application.NamedServiceReference `sep:"none" help:"Service reference to add/update in the form name=kind/target-name. Repeat the flag to pass more than one service."`
DeleteService []string `help:"Service reference names to remove."`
RetryRelease *bool `help:"Retries release for the application." placeholder:"false"`
RetryBuild *bool `help:"Retries build for the application if set to true." placeholder:"false"`
Pause *bool `negatable:"" help:"Pause or unpause the application. Pausing stops all costs."`
GitInformationServiceURL string `help:"URL of the git information service." default:"https://git-info.deplo.io" env:"GIT_INFORMATION_SERVICE_URL" hidden:""`
SkipRepoAccessCheck bool `help:"Skip the git repository access check." default:"false"`
Debug bool `help:"Enable debug messages." default:"false"`
Language *string `help:"${app_language_help} Possible values: ${enum}" enum:"ruby,php,python,golang,nodejs,static,"`
DockerfileBuild dockerfileBuild `embed:""`
BuildpackStack *string `help:"${app_buildpack_stack_help} Possible values: ${enum}" enum:"paketo,heroku,"`
}

type gitConfig struct {
Expand Down Expand Up @@ -366,7 +366,7 @@ func (cmd *applicationCmd) applyUpdates(app *apps.Application) {
}

if len(cmd.Service) > 0 || len(cmd.DeleteService) > 0 {
toAdd := application.ServicesFromMap(cmd.Service, app.Namespace)
toAdd := application.ServicesFromReferences(cmd.Service, app.Namespace)
app.Spec.ForProvider.Services = application.UpdateServices(
app.Spec.ForProvider.Services, toAdd, cmd.DeleteService, cmd.Writer,
)
Expand Down
4 changes: 2 additions & 2 deletions update/application_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,10 +790,10 @@ func TestApplication(t *testing.T) {
ResourceCmd: ResourceCmd{
Name: existingApp.Name,
},
Service: func() application.ServiceMap {
Service: func() []application.NamedServiceReference {
ref := application.TypedReference{}
ref.UnmarshalText([]byte("keyvaluestore/my-kvs"))
return application.ServiceMap{"cache": ref}
return []application.NamedServiceReference{{Name: "cache", Target: ref}}
}(),
},
checkApp: func(t *testing.T, cmd applicationCmd, orig, updated *apps.Application) {
Expand Down
Loading