From 047b7942766faf64143248b405d8cf3db2a21ca5 Mon Sep 17 00:00:00 2001 From: Saad Assaf Date: Tue, 1 Sep 2026 10:45:20 +0200 Subject: [PATCH] fix: allow multiple --service flags with the same name in one command - UpdateServices now matches by name and kind not name alone, so updating an app across multiple commands keeps both entries. - --service changed from a map-typed flag to a slice-typed one --- create/application.go | 44 ++++++++++----------- create/application_test.go | 10 ++--- internal/application/service.go | 36 +++++++++++++----- internal/application/service_test.go | 57 ++++++++++++++++++++++++---- update/application.go | 34 ++++++++--------- update/application_test.go | 4 +- 6 files changed, 122 insertions(+), 63 deletions(-) diff --git a/create/application.go b/create/application.go index f26837ec..57366a7e 100644 --- a/create/application.go +++ b/create/application.go @@ -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 { @@ -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, diff --git a/create/application_test.go b/create/application_test.go index 2ce4a87c..6deb1164 100644 --- a/create/application_test.go +++ b/create/application_test.go @@ -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, }, diff --git a/internal/application/service.go b/internal/application/service.go index 2ab6380c..e2bdd508 100644 --- a/internal/application/service.go +++ b/internal/application/service.go @@ -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, }) } @@ -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 diff --git a/internal/application/service_test.go b/internal/application/service_test.go index bdf83a62..94da2260 100644 --- a/internal/application/service_test.go +++ b/internal/application/service_test.go @@ -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{} @@ -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{ { @@ -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{ @@ -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) }) } @@ -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) { diff --git a/update/application.go b/update/application.go index 4a516e7e..1f248bbe 100644 --- a/update/application.go +++ b/update/application.go @@ -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 { @@ -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, ) diff --git a/update/application_test.go b/update/application_test.go index 84d61372..35c8e1d4 100644 --- a/update/application_test.go +++ b/update/application_test.go @@ -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) {