diff --git a/.report/STACKITCRE-382.md b/.report/STACKITCRE-382.md new file mode 100644 index 000000000..5992eb82e --- /dev/null +++ b/.report/STACKITCRE-382.md @@ -0,0 +1,162 @@ +# Code Review Report - STACKITCRE-382 + +**Rama analizada:** `feat/STACKITCRE-382-registry` (comparada contra `origin/main`) +**Fecha:** 2026-10-23 +**Revisor:** Senior Code Reviewer + +--- + +## Resumen Ejecutivo + +Se revisó la implementación de los nuevos comandos para Container Registry (`stackit registry artifactory [create|delete|describe|list|update]`), la configuración del cliente API y la documentación generada. + +La estructura general y la integración con Cobra y el SDK de STACKIT son sólidas y siguen los patrones base de `stackit-cli`. Todos los hallazgos de código (ítems 1 al 8) han sido corregidos y validados con tests unitarios y linter. El ítem 9 (enmienda de commit) queda reservado para la etapa de commit/rebase interactivo cuando el usuario lo autorice. + +--- + +## Estado de Resolución + +- [x] **Ítem 1 (Major):** Validación conjunta obligatoria de credenciales proxy (`MarkFlagsRequiredTogether`) en `update.go`. +- [x] **Ítem 2 (Major):** Manejo seguro de secretos con `flags.SecretFlag`, `SecretFlagToStringPointer` y `json:"-"` en `create.go` y `update.go`. +- [x] **Ítem 3 (Major):** Paginación completa con seguimiento de `NextPageToken` y respeto de `--limit` en `list.go`. +- [x] **Ítem 4 (Minor):** Soporte para `--registry-custom-endpoint` en `config set`, `config unset` y `profile.json`. +- [x] **Ítem 5 (Minor):** Verificación de respuesta nil en `GetArtifactoryName` y suite de tests unitarios en `utils_test.go`. +- [x] **Ítem 6 (Minor):** Captura de entidad actualizada retornada por los `WaitHandler` en `create.go` y `update.go`. +- [x] **Ítem 7 (Suggestion):** Formato homogéneo de fecha `time.DateTime` en la tabla de `describe.go`. +- [x] **Ítem 8 (Suggestion):** Ampliación de la cobertura de pruebas unitarias en `update_test.go`. +- [ ] **Ítem 9 (Suggestion):** Corrección tipográfica en mensaje de commit `92766842` (`chroe` -> `chore`) pendiente de rebase/amend cuando el usuario lo indique. + +--- + +## Hallazgos + +### 1. Descarte silencioso de credenciales por omisión de `MarkFlagsRequiredTogether` +- **Severidad:** `Major` +- **Ubicación:** [update.go:103-108](file:///Users/catalan/Project/STACKIT/stackit-cli/internal/cmd/registry/artifactory/update/update.go#L103-L108), [update.go:148-153](file:///Users/catalan/Project/STACKIT/stackit-cli/internal/cmd/registry/artifactory/update/update.go#L148-L153) +- **Problema concreto:** En `update.go`, no se vinculó `cmd.MarkFlagsRequiredTogether(proxyAccessKeyFlag, proxyAccessSecretFlag)` como sí se hizo en `create.go`. Si un usuario ejecuta el comando especificando únicamente `--proxy-access-key` (o `--proxy-access-secret`), la validación pasa con éxito. No obstante, en `buildRequest` la condición `model.ProxyAccessKey != nil && model.ProxyAccessSecret != nil` se evalúa como falsa, por lo que `proxyReq.Credentials` queda `nil`. La petición se envía a la API sin credenciales y el CLI emite `"Updated artifactory..."`, descartando silenciosamente la credencial provista por el usuario sin ningún aviso o error. +- **Sugerencia de corrección:** + Configurar la regla de flags en `configureFlags`: + ```go + func configureFlags(cmd *cobra.Command) { + cmd.Flags().String(proxyUrlFlag, "", "Upstream registry URL to proxy") + cmd.Flags().String(proxyDescriptionFlag, "", "Description of the proxy registry") + cmd.Flags().String(proxyAccessKeyFlag, "", "Access key for upstream proxy registry authentication") + cmd.Flags().String(proxyAccessSecretFlag, "", "Access secret for upstream proxy registry authentication") + + cmd.MarkFlagsRequiredTogether(proxyAccessKeyFlag, proxyAccessSecretFlag) + } + ``` + +--- + +### 2. Exposición de secretos en línea de comandos, logs de depuración y falta de soporte para `@file`/`stdin` +- **Severidad:** `Major` +- **Ubicación:** [create.go:120](file:///Users/catalan/Project/STACKIT/stackit-cli/internal/cmd/registry/artifactory/create/create.go#L120), [update.go:107](file:///Users/catalan/Project/STACKIT/stackit-cli/internal/cmd/registry/artifactory/update/update.go#L107), [create.go:33-41](file:///Users/catalan/Project/STACKIT/stackit-cli/internal/cmd/registry/artifactory/create/create.go#L33-L41) +- **Problema concreto:** El flag `--proxy-access-secret` se registró como un flag string plano ordinario (`cmd.Flags().String`). En todo el CLI (`alb`, `cdn`, `load-balancer`, `intake`), los secretos deben configurarse mediante `flags.SecretFlag(proxyAccessSecretFlag, params)` y extraerse con `flags.SecretFlagToStringPointer`. La implementación actual causa: + 1. Exposición del secreto en el historial de shell y en la lista de procesos (`ps aux`). + 2. Imposibilidad de suministrar el secreto de forma segura mediante un archivo (`--proxy-access-secret @./secret.txt`) o stdin. + 3. Fuga del secreto en texto plano en los logs de depuración: `p.DebugInputModel(model)` convierte `inputModel` a JSON y luego a mapa; al ser un campo exportado sin ofuscación, el secreto se imprime en los logs de debug. +- **Sugerencia de corrección:** + Usar `flags.SecretFlag` y proteger el modelo: + ```go + // En configureFlags(cmd *cobra.Command, params *types.CmdParams): + secretFlag := flags.SecretFlag(proxyAccessSecretFlag, params) + cmd.Flags().Var(secretFlag, proxyAccessSecretFlag, secretFlag.Usage()) + + // En parseInput: + ProxyAccessSecret: flags.SecretFlagToStringPointer(p, cmd, proxyAccessSecretFlag), + + // En inputModel: + ProxyAccessSecret *string `json:"-"` + ``` + +--- + +### 3. Falta de paginación en `list` (pérdida de resultados) +- **Severidad:** `Major` +- **Ubicación:** [list.go:62-73](file:///Users/catalan/Project/STACKIT/stackit-cli/internal/cmd/registry/artifactory/list/list.go#L62-L73) +- **Problema concreto:** `ApiListArtifactoriesRequest` admite `NextPageToken` y `PageSize`, y la API responde con un `NextPageToken`. Actualmente, `list.go` solo ejecuta un único llamado: + ```go + req := buildRequest(ctx, model, apiClient) + resp, err := req.Execute() + ``` + Si el proyecto cuenta con más elementos que el tamaño de página por defecto o si el usuario indica un `--limit` superior a una sola página, los elementos posteriores nunca se obtienen ni se muestran, reportando una lista incompleta de artifactories. +- **Sugerencia de corrección:** + Implementar el bucle de paginación siguiendo `NextPageToken` hasta alcanzar el `limit` o hasta que no haya más páginas, siguiendo el patrón de servicios como `beta/cdn/distribution/list/list.go`. + +--- + +### 4. Inconsistencia de configuración: endpoint de registry no expuesto en `config set` / `config unset` +- **Severidad:** `Minor` +- **Ubicación:** [config.go:62](file:///Users/catalan/Project/STACKIT/stackit-cli/internal/pkg/config/config.go#L62), [set.go:58](file:///Users/catalan/Project/STACKIT/stackit-cli/internal/cmd/config/set/set.go#L58), [unset.go:61](file:///Users/catalan/Project/STACKIT/stackit-cli/internal/cmd/config/unset/unset.go#L61) +- **Problema concreto:** Se añadió `RegistryCustomEndpointKey` a `internal/pkg/config/config.go` y se consume en `internal/pkg/services/registry/client/client.go`. Sin embargo, no se crearon los flags correspondientes `--registry-custom-endpoint` ni en `stackit config set` ni en `stackit config unset`, ni se agregó al template de perfiles `profile.json`. En consecuencia, ningún usuario puede configurar ni eliminar dicho endpoint mediante los comandos estándar de configuración. +- **Sugerencia de corrección:** + Agregar el flag `registryCustomEndpointFlag = "registry-custom-endpoint"` y su enlace con viper en `internal/cmd/config/set/set.go` e `internal/cmd/config/unset/unset.go`. + +--- + +### 5. Falta de verificación de nil en `GetArtifactoryName` y ausencia total de `utils_test.go` +- **Severidad:** `Minor` +- **Ubicación:** [utils.go:10-15](file:///Users/catalan/Project/STACKIT/stackit-cli/internal/pkg/services/registry/utils/utils.go#L10-L15) +- **Problema concreto:** La función `GetArtifactoryName` desreferencia directamente `resp.Name` tras el llamado API sin verificar previamente si `resp != nil`. Además, el paquete `internal/pkg/services/registry/utils` no cuenta con archivo de tests unitarios (`utils_test.go`), a diferencia de prácticamente todos los demás servicios bajo `internal/pkg/services/*/utils/`. +- **Sugerencia de corrección:** + Agregar validación de nil y crear `utils_test.go` con mocks del servicio: + ```go + func GetArtifactoryName(ctx context.Context, apiClient registry.DefaultAPI, projectId, region, artifactoryId string) (string, error) { + resp, err := apiClient.GetArtifactory(ctx, projectId, region, artifactoryId).Execute() + if err != nil { + return "", fmt.Errorf("get artifactory: %w", err) + } + if resp == nil { + return "", fmt.Errorf("get artifactory: empty response") + } + return resp.Name, nil + } + ``` + +--- + +### 6. Descarte de la respuesta actualizada del `WaitHandler` en modo sincrónico +- **Severidad:** `Minor` +- **Ubicación:** [create.go:99](file:///Users/catalan/Project/STACKIT/stackit-cli/internal/cmd/registry/artifactory/create/create.go#L99), [update.go:88](file:///Users/catalan/Project/STACKIT/stackit-cli/internal/cmd/registry/artifactory/update/update.go#L88) +- **Problema concreto:** Al completarse la espera (`wait.CreateArtifactoryWaitHandler` o `wait.UpdateArtifactoryWaitHandler`), el handler retorna la entidad `*registry.Artifactory` con su estado actualizado. Sin embargo, el código ignora dicho retorno (`_, err = ...`) y le pasa a `outputResult` el `resp` inicial devuelto por la llamada de creación/actualización. En formatos de salida como JSON (`-o json`), la respuesta puede reflejar estados intermedios en vez del estado consolidado tras la espera. +- **Sugerencia de corrección:** + Capturar la respuesta del wait handler: + ```go + err := spinner.Run(params.Printer, "Creating artifactory", func() error { + resp, err = wait.CreateArtifactoryWaitHandler(ctx, apiClient.DefaultAPI, model.ProjectId, model.Region, artifactoryId).WaitWithContext(ctx) + return err + }) + ``` + +--- + +### 7. Formato crudo de fecha en salida de tabla de `describe` +- **Severidad:** `Suggestion` +- **Ubicación:** [describe.go:108](file:///Users/catalan/Project/STACKIT/stackit-cli/internal/cmd/registry/artifactory/describe/describe.go#L108) +- **Problema concreto:** `table.AddRow("CREATED AT", artifactory.CreateTime.String())` invoca el método por defecto de Go `time.Time.String()`, imprimiendo timestamps con sufijos de zona y reloj monotónico desalineados con el resto de la CLI. En toda la aplicación se utiliza `artifactory.CreateTime.Format(time.DateTime)` o `time.RFC3339`. +- **Sugerencia de corrección:** + ```go + table.AddRow("CREATED AT", artifactory.CreateTime.Format(time.DateTime)) + ``` + +--- + +### 8. Cobertura incompleta de casos de prueba en `update_test.go` +- **Severidad:** `Suggestion` +- **Ubicación:** [update_test.go:74-140](file:///Users/catalan/Project/STACKIT/stackit-cli/internal/cmd/registry/artifactory/update/update_test.go#L74-L140) +- **Problema concreto:** La suite de pruebas de `update` solo cubre el escenario en el que se modifica `--proxy-url`. No existen casos de prueba para verificar: + - Actualización de `--proxy-description`. + - Actualización con credenciales (`--proxy-access-key` y `--proxy-access-secret`). + - Validación de combinaciones inválidas (por ejemplo, clave sin secreto). +- **Sugerencia de corrección:** + Añadir casos de prueba para cada bandera de proxy en `TestParseInput` y `TestBuildRequest`. + +--- + +### 9. Typo en mensaje de commit (Conventional Commits) +- **Severidad:** `Suggestion` +- **Ubicación:** Commit `92766842` +- **Problema concreto:** El commit `92766842` contiene un error tipográfico en el tipo de cambio: `chroe(cli): revert bump` en lugar de `chore(cli): revert bump`. +- **Sugerencia de corrección:** + Corregir mediante rebase interactivo (`git commit --amend`) antes de integrar a la rama principal. diff --git a/docs/stackit_config_set.md b/docs/stackit_config_set.md index 9f21f4095..26588849f 100644 --- a/docs/stackit_config_set.md +++ b/docs/stackit_config_set.md @@ -54,6 +54,7 @@ stackit config set [flags] --postgresflex-custom-endpoint string PostgreSQL Flex API base URL, used in calls to this API --rabbitmq-custom-endpoint string RabbitMQ API base URL, used in calls to this API --redis-custom-endpoint string Redis API base URL, used in calls to this API + --registry-custom-endpoint string Registry API base URL, used in calls to this API --resource-manager-custom-endpoint string Resource Manager API base URL, used in calls to this API --runcommand-custom-endpoint string Run Command API base URL, used in calls to this API --secrets-manager-custom-endpoint string Secrets Manager API base URL, used in calls to this API diff --git a/docs/stackit_config_unset.md b/docs/stackit_config_unset.md index 5f64ad2e4..c99f5869b 100644 --- a/docs/stackit_config_unset.md +++ b/docs/stackit_config_unset.md @@ -56,6 +56,7 @@ stackit config unset [flags] --rabbitmq-custom-endpoint RabbitMQ API base URL. If unset, uses the default base URL --redis-custom-endpoint Redis API base URL. If unset, uses the default base URL --region Region + --registry-custom-endpoint Registry API base URL. If unset, uses the default base URL --resource-manager-custom-endpoint Resource Manager API base URL. If unset, uses the default base URL --runcommand-custom-endpoint Server Command base URL. If unset, uses the default base URL --secrets-manager-custom-endpoint Secrets Manager API base URL. If unset, uses the default base URL diff --git a/docs/stackit_registry_artifactory_create.md b/docs/stackit_registry_artifactory_create.md index 6cab764cc..2707c3efc 100644 --- a/docs/stackit_registry_artifactory_create.md +++ b/docs/stackit_registry_artifactory_create.md @@ -29,7 +29,7 @@ stackit registry artifactory create [flags] -h, --help Help for "stackit registry artifactory create" -n, --name string Artifactory name --proxy-access-key string Access key for upstream proxy registry authentication - --proxy-access-secret string Access secret for upstream proxy registry authentication + --proxy-access-secret string Proxy-Access-Secret. Can be a string (deprecated) or a file path, if prefixed with '@' (example: @./secret.txt). Will be read from stdin when empty. --proxy-description string Description of the proxy registry --proxy-registry-type string Registry type of the upstream proxy (e.g., docker-hub, harbor) --proxy-url string Upstream registry URL to proxy diff --git a/docs/stackit_registry_artifactory_update.md b/docs/stackit_registry_artifactory_update.md index 9846c2756..71a370967 100644 --- a/docs/stackit_registry_artifactory_update.md +++ b/docs/stackit_registry_artifactory_update.md @@ -22,7 +22,7 @@ stackit registry artifactory update ARTIFACTORY_ID [flags] ``` -h, --help Help for "stackit registry artifactory update" --proxy-access-key string Access key for upstream proxy registry authentication - --proxy-access-secret string Access secret for upstream proxy registry authentication + --proxy-access-secret string Proxy-Access-Secret. Can be a string (deprecated) or a file path, if prefixed with '@' (example: @./secret.txt). Will be read from stdin when empty. --proxy-description string Description of the proxy registry --proxy-url string Upstream registry URL to proxy ``` diff --git a/internal/cmd/config/profile/import/template/profile.json b/internal/cmd/config/profile/import/template/profile.json index ed2702e7e..c5c056795 100644 --- a/internal/cmd/config/profile/import/template/profile.json +++ b/internal/cmd/config/profile/import/template/profile.json @@ -20,6 +20,7 @@ "project_name": "", "rabbitmq_custom_endpoint": "", "redis_custom_endpoint": "", + "registry_custom_endpoint": "", "resource_manager_custom_endpoint": "", "runcommand_custom_endpoint": "", "secrets_manager_custom_endpoint": "", diff --git a/internal/cmd/config/set/set.go b/internal/cmd/config/set/set.go index 194291a3d..8a13e0c9e 100644 --- a/internal/cmd/config/set/set.go +++ b/internal/cmd/config/set/set.go @@ -57,6 +57,7 @@ const ( sfsCustomEndpointFlag = "sfs-custom-endpoint" cdnCustomEndpointFlag = "cdn-custom-endpoint" vpnCustomEndpointFlag = "vpn-custom-endpoint" + registryCustomEndpointFlag = "registry-custom-endpoint" ) type inputModel struct { @@ -180,6 +181,7 @@ func configureFlags(cmd *cobra.Command) { cmd.Flags().String(sfsCustomEndpointFlag, "", "SFS API base URL, used in calls to this API") cmd.Flags().String(cdnCustomEndpointFlag, "", "CDN API base URL, used in calls to this API") cmd.Flags().String(vpnCustomEndpointFlag, "", "VPN API base URL, used in calls to this API") + cmd.Flags().String(registryCustomEndpointFlag, "", "Registry API base URL, used in calls to this API") err := viper.BindPFlag(config.SessionTimeLimitKey, cmd.Flags().Lookup(sessionTimeLimitFlag)) cobra.CheckErr(err) @@ -256,6 +258,8 @@ func configureFlags(cmd *cobra.Command) { cobra.CheckErr(err) err = viper.BindPFlag(config.VPNCustomEndpointKey, cmd.Flags().Lookup(vpnCustomEndpointFlag)) cobra.CheckErr(err) + err = viper.BindPFlag(config.RegistryCustomEndpointKey, cmd.Flags().Lookup(registryCustomEndpointFlag)) + cobra.CheckErr(err) } func parseInput(p *print.Printer, cmd *cobra.Command, _ []string) (*inputModel, error) { diff --git a/internal/cmd/config/unset/unset.go b/internal/cmd/config/unset/unset.go index 5aff526eb..d5314e51c 100644 --- a/internal/cmd/config/unset/unset.go +++ b/internal/cmd/config/unset/unset.go @@ -60,6 +60,7 @@ const ( logsCustomEndpointFlag = "logs-custom-endpoint" cdnCustomEndpointFlag = "cdn-custom-endpoint" vpnCustomEndpointFlag = "vpn-custom-endpoint" + registryCustomEndpointFlag = "registry-custom-endpoint" ) var ( @@ -113,6 +114,7 @@ type inputModel struct { LogsCustomEndpoint bool CDNCustomEndpoint bool VpnCustomEndpoint bool + RegistryCustomEndpoint bool } func NewCmd(params *types.CmdParams) *cobra.Command { @@ -266,6 +268,9 @@ func NewCmd(params *types.CmdParams) *cobra.Command { if model.VpnCustomEndpoint { viper.Set(config.VPNCustomEndpointKey, "") } + if model.RegistryCustomEndpoint { + viper.Set(config.RegistryCustomEndpointKey, "") + } err := config.Write() if err != nil { @@ -324,6 +329,7 @@ func configureFlags(cmd *cobra.Command) { cmd.Flags().Bool(sfsCustomEndpointFlag, false, "SFS API base URL. If unset, uses the default base URL") cmd.Flags().Bool(cdnCustomEndpointFlag, false, "Custom CDN endpoint URL. If unset, uses the default base URL") cmd.Flags().Bool(vpnCustomEndpointFlag, false, "VPN API base URL. If unset, uses the default base URL") + cmd.Flags().Bool(registryCustomEndpointFlag, false, "Registry API base URL. If unset, uses the default base URL") } func parseInput(p *print.Printer, cmd *cobra.Command) *inputModel { @@ -373,6 +379,7 @@ func parseInput(p *print.Printer, cmd *cobra.Command) *inputModel { LogsCustomEndpoint: flags.FlagToBoolValue(p, cmd, logsCustomEndpointFlag), CDNCustomEndpoint: flags.FlagToBoolValue(p, cmd, cdnCustomEndpointFlag), VpnCustomEndpoint: flags.FlagToBoolValue(p, cmd, vpnCustomEndpointFlag), + RegistryCustomEndpoint: flags.FlagToBoolValue(p, cmd, registryCustomEndpointFlag), } p.DebugInputModel(model) diff --git a/internal/cmd/config/unset/unset_test.go b/internal/cmd/config/unset/unset_test.go index 46a41ee21..ccdad702e 100644 --- a/internal/cmd/config/unset/unset_test.go +++ b/internal/cmd/config/unset/unset_test.go @@ -52,6 +52,7 @@ func fixtureFlagValues(mods ...func(flagValues map[string]bool)) map[string]bool logsCustomEndpointFlag: true, cdnCustomEndpointFlag: true, vpnCustomEndpointFlag: true, + registryCustomEndpointFlag: true, } for _, mod := range mods { mod(flagValues) @@ -102,6 +103,7 @@ func fixtureInputModel(mods ...func(model *inputModel)) *inputModel { LogsCustomEndpoint: true, CDNCustomEndpoint: true, VpnCustomEndpoint: true, + RegistryCustomEndpoint: true, } for _, mod := range mods { mod(model) @@ -168,6 +170,7 @@ func TestParseInput(t *testing.T) { model.LogsCustomEndpoint = false model.CDNCustomEndpoint = false model.VpnCustomEndpoint = false + model.RegistryCustomEndpoint = false }), }, { diff --git a/internal/cmd/registry/artifactory/create/create.go b/internal/cmd/registry/artifactory/create/create.go index 62e5d0022..776a30690 100644 --- a/internal/cmd/registry/artifactory/create/create.go +++ b/internal/cmd/registry/artifactory/create/create.go @@ -37,7 +37,7 @@ type inputModel struct { ProxyRegistryType *string ProxyDescription *string ProxyAccessKey *string - ProxyAccessSecret *string + ProxyAccessSecret *string `json:"-"` } func NewCmd(params *types.CmdParams) *cobra.Command { @@ -96,8 +96,12 @@ func NewCmd(params *types.CmdParams) *cobra.Command { // Wait for async operation, if async mode not enabled if !model.Async { err := spinner.Run(params.Printer, "Creating artifactory", func() error { - _, err = wait.CreateArtifactoryWaitHandler(ctx, apiClient.DefaultAPI, model.ProjectId, model.Region, artifactoryId).WaitWithContext(ctx) - return err + waitResp, waitErr := wait.CreateArtifactoryWaitHandler(ctx, apiClient.DefaultAPI, model.ProjectId, model.Region, artifactoryId).WaitWithContext(ctx) + if waitErr != nil { + return waitErr + } + resp = waitResp + return nil }) if err != nil { return fmt.Errorf("wait for artifactory creation: %w", err) @@ -107,17 +111,19 @@ func NewCmd(params *types.CmdParams) *cobra.Command { return outputResult(params.Printer, model, projectLabel, artifactoryId, resp) }, } - configureFlags(cmd) + configureFlags(cmd, params) return cmd } -func configureFlags(cmd *cobra.Command) { +func configureFlags(cmd *cobra.Command, params *types.CmdParams) { cmd.Flags().StringP(nameFlag, "n", "", "Artifactory name") cmd.Flags().String(proxyUrlFlag, "", "Upstream registry URL to proxy") cmd.Flags().String(proxyRegistryTypeFlag, "", "Registry type of the upstream proxy (e.g., docker-hub, harbor)") cmd.Flags().String(proxyDescriptionFlag, "", "Description of the proxy registry") cmd.Flags().String(proxyAccessKeyFlag, "", "Access key for upstream proxy registry authentication") - cmd.Flags().String(proxyAccessSecretFlag, "", "Access secret for upstream proxy registry authentication") + + secret := flags.SecretFlag(proxyAccessSecretFlag, params) + cmd.Flags().Var(secret, proxyAccessSecretFlag, secret.Usage()) err := flags.MarkFlagsRequired(cmd, nameFlag) cobra.CheckErr(err) @@ -136,7 +142,7 @@ func parseInput(p *print.Printer, cmd *cobra.Command, _ []string) (*inputModel, proxyRegistryType := flags.FlagToStringPointer(p, cmd, proxyRegistryTypeFlag) proxyDesc := flags.FlagToStringPointer(p, cmd, proxyDescriptionFlag) proxyKey := flags.FlagToStringPointer(p, cmd, proxyAccessKeyFlag) - proxySecret := flags.FlagToStringPointer(p, cmd, proxyAccessSecretFlag) + proxySecret := flags.SecretFlagToStringPointer(p, cmd, proxyAccessSecretFlag) hasProxyDetails := proxyDesc != nil || proxyKey != nil || proxySecret != nil if hasProxyDetails && (proxyUrl == nil || proxyRegistryType == nil) { diff --git a/internal/cmd/registry/artifactory/describe/describe.go b/internal/cmd/registry/artifactory/describe/describe.go index def54a318..52f0ba375 100644 --- a/internal/cmd/registry/artifactory/describe/describe.go +++ b/internal/cmd/registry/artifactory/describe/describe.go @@ -3,6 +3,7 @@ package describe import ( "context" "fmt" + "time" "github.com/spf13/cobra" registry "github.com/stackitcloud/stackit-sdk-go/services/registry/v1api" @@ -105,7 +106,7 @@ func outputResult(p *print.Printer, outputFormat string, artifactory *registry.A table.AddSeparator() table.AddRow("CONSUMED QUOTA BYTES", artifactory.ConsumedQuotaBytes) table.AddSeparator() - table.AddRow("CREATED AT", artifactory.CreateTime.String()) + table.AddRow("CREATED AT", artifactory.CreateTime.Format(time.DateTime)) if artifactory.Proxy != nil { table.AddSeparator() table.AddRow("PROXY URL", artifactory.Proxy.Url) diff --git a/internal/cmd/registry/artifactory/list/list.go b/internal/cmd/registry/artifactory/list/list.go index 66f1a9a4c..87829e49b 100644 --- a/internal/cmd/registry/artifactory/list/list.go +++ b/internal/cmd/registry/artifactory/list/list.go @@ -3,6 +3,7 @@ package list import ( "context" "fmt" + "math" "github.com/spf13/cobra" registry "github.com/stackitcloud/stackit-sdk-go/services/registry/v1api" @@ -20,7 +21,8 @@ import ( ) const ( - limitFlag = "limit" + limitFlag = "limit" + maxPageSize = 100 ) type inputModel struct { @@ -59,16 +61,9 @@ func NewCmd(params *types.CmdParams) *cobra.Command { } // Call API - req := buildRequest(ctx, model, apiClient) - resp, err := req.Execute() + artifactories, err := fetchArtifactories(ctx, model, apiClient) if err != nil { - return fmt.Errorf("get artifactories: %w", err) - } - artifactories := resp.Artifactories - - // Truncate output - if model.Limit != nil && len(artifactories) > int(*model.Limit) { - artifactories = artifactories[:*model.Limit] + return err } projectLabel, err := projectname.GetProjectName(ctx, params.Printer, params.CliVersion, cmd) @@ -112,8 +107,51 @@ func parseInput(p *print.Printer, cmd *cobra.Command, _ []string) (*inputModel, return &model, nil } -func buildRequest(ctx context.Context, model *inputModel, apiClient *registry.APIClient) registry.ApiListArtifactoriesRequest { - return apiClient.DefaultAPI.ListArtifactories(ctx, model.ProjectId, model.Region) +func buildRequest(ctx context.Context, model *inputModel, apiClient *registry.APIClient, pageToken string, pageSize int32) registry.ApiListArtifactoriesRequest { + req := apiClient.DefaultAPI.ListArtifactories(ctx, model.ProjectId, model.Region) + if pageToken != "" { + req = req.NextPageToken(pageToken) + } + if pageSize > 0 { + req = req.PageSize(pageSize) + } + return req +} + +func fetchArtifactories(ctx context.Context, model *inputModel, apiClient *registry.APIClient) ([]registry.Artifactory, error) { + var pageToken string + artifactories := make([]registry.Artifactory, 0) + received := int64(0) + limit := int64(math.MaxInt64) + if model.Limit != nil { + limit = *model.Limit + } + for { + want := min(int64(maxPageSize), limit-received) + request := buildRequest(ctx, model, apiClient, pageToken, int32(want)) + response, err := request.Execute() + if err != nil { + return nil, fmt.Errorf("get artifactories: %w", err) + } + if response == nil { + return nil, fmt.Errorf("get artifactories: empty response") + } + if response.Artifactories != nil { + artifactories = append(artifactories, response.Artifactories...) + } + pageToken = "" + if response.NextPageToken != nil { + pageToken = *response.NextPageToken + } + received += want + if pageToken == "" || received >= limit { + break + } + } + if model.Limit != nil && len(artifactories) > int(*model.Limit) { + artifactories = artifactories[:*model.Limit] + } + return artifactories, nil } func outputResult(p *print.Printer, outputFormat, projectLabel string, artifactories []registry.Artifactory) error { diff --git a/internal/cmd/registry/artifactory/list/list_test.go b/internal/cmd/registry/artifactory/list/list_test.go index e0b73b7ed..3c8b12f7e 100644 --- a/internal/cmd/registry/artifactory/list/list_test.go +++ b/internal/cmd/registry/artifactory/list/list_test.go @@ -54,7 +54,7 @@ func fixtureInputModel(mods ...func(model *inputModel)) *inputModel { } func fixtureRequest(mods ...func(request *registry.ApiListArtifactoriesRequest)) registry.ApiListArtifactoriesRequest { - request := testClient.DefaultAPI.ListArtifactories(testCtx, testProjectId, testRegion) + request := testClient.DefaultAPI.ListArtifactories(testCtx, testProjectId, testRegion).PageSize(maxPageSize) for _, mod := range mods { mod(&request) } @@ -137,18 +137,30 @@ func TestBuildRequest(t *testing.T) { tests := []struct { description string model *inputModel + pageToken string + pageSize int32 expectedRequest registry.ApiListArtifactoriesRequest }{ { description: "base", model: fixtureInputModel(), + pageSize: maxPageSize, expectedRequest: fixtureRequest(), }, + { + description: "with page token", + model: fixtureInputModel(), + pageToken: "next-token", + pageSize: maxPageSize, + expectedRequest: fixtureRequest(func(request *registry.ApiListArtifactoriesRequest) { + *request = request.NextPageToken("next-token") + }), + }, } for _, tt := range tests { t.Run(tt.description, func(t *testing.T) { - request := buildRequest(testCtx, tt.model, testClient) + request := buildRequest(testCtx, tt.model, testClient, tt.pageToken, tt.pageSize) diff := cmp.Diff(request, tt.expectedRequest, cmp.AllowUnexported(tt.expectedRequest, registry.DefaultAPIService{}), diff --git a/internal/cmd/registry/artifactory/update/update.go b/internal/cmd/registry/artifactory/update/update.go index 10cbdd8f5..8db95bd2e 100644 --- a/internal/cmd/registry/artifactory/update/update.go +++ b/internal/cmd/registry/artifactory/update/update.go @@ -36,7 +36,7 @@ type inputModel struct { ProxyUrl *string ProxyDescription *string ProxyAccessKey *string - ProxyAccessSecret *string + ProxyAccessSecret *string `json:"-"` } func NewCmd(params *types.CmdParams) *cobra.Command { @@ -85,8 +85,12 @@ func NewCmd(params *types.CmdParams) *cobra.Command { // Wait for async operation, if async mode not enabled if !model.Async { err := spinner.Run(params.Printer, "Updating artifactory", func() error { - _, err = wait.UpdateArtifactoryWaitHandler(ctx, apiClient.DefaultAPI, model.ProjectId, model.Region, model.ArtifactoryId).WaitWithContext(ctx) - return err + waitResp, waitErr := wait.UpdateArtifactoryWaitHandler(ctx, apiClient.DefaultAPI, model.ProjectId, model.Region, model.ArtifactoryId).WaitWithContext(ctx) + if waitErr != nil { + return waitErr + } + resp = waitResp + return nil }) if err != nil { return fmt.Errorf("wait for artifactory update: %w", err) @@ -96,15 +100,19 @@ func NewCmd(params *types.CmdParams) *cobra.Command { return outputResult(params.Printer, model, artifactoryLabel, resp) }, } - configureFlags(cmd) + configureFlags(cmd, params) return cmd } -func configureFlags(cmd *cobra.Command) { +func configureFlags(cmd *cobra.Command, params *types.CmdParams) { cmd.Flags().String(proxyUrlFlag, "", "Upstream registry URL to proxy") cmd.Flags().String(proxyDescriptionFlag, "", "Description of the proxy registry") cmd.Flags().String(proxyAccessKeyFlag, "", "Access key for upstream proxy registry authentication") - cmd.Flags().String(proxyAccessSecretFlag, "", "Access secret for upstream proxy registry authentication") + + secret := flags.SecretFlag(proxyAccessSecretFlag, params) + cmd.Flags().Var(secret, proxyAccessSecretFlag, secret.Usage()) + + cmd.MarkFlagsRequiredTogether(proxyAccessKeyFlag, proxyAccessSecretFlag) } func parseInput(p *print.Printer, cmd *cobra.Command, inputArgs []string) (*inputModel, error) { @@ -118,7 +126,7 @@ func parseInput(p *print.Printer, cmd *cobra.Command, inputArgs []string) (*inpu proxyUrl := flags.FlagToStringPointer(p, cmd, proxyUrlFlag) proxyDesc := flags.FlagToStringPointer(p, cmd, proxyDescriptionFlag) proxyKey := flags.FlagToStringPointer(p, cmd, proxyAccessKeyFlag) - proxySecret := flags.FlagToStringPointer(p, cmd, proxyAccessSecretFlag) + proxySecret := flags.SecretFlagToStringPointer(p, cmd, proxyAccessSecretFlag) if proxyUrl == nil && proxyDesc == nil && proxyKey == nil && proxySecret == nil { return nil, &cliErr.EmptyUpdateError{} diff --git a/internal/cmd/registry/artifactory/update/update_test.go b/internal/cmd/registry/artifactory/update/update_test.go index 55159f5fe..f0d16e7cc 100644 --- a/internal/cmd/registry/artifactory/update/update_test.go +++ b/internal/cmd/registry/artifactory/update/update_test.go @@ -23,11 +23,14 @@ const ( type testCtxKey struct{} var ( - testCtx = context.WithValue(context.Background(), testCtxKey{}, "foo") - testClient = ®istry.APIClient{DefaultAPI: ®istry.DefaultAPIService{}} - testProjectId = uuid.NewString() - testArtifactoryId = uuid.NewString() - testProxyUrl = "https://registry-1.docker.io" + testCtx = context.WithValue(context.Background(), testCtxKey{}, "foo") + testClient = ®istry.APIClient{DefaultAPI: ®istry.DefaultAPIService{}} + testProjectId = uuid.NewString() + testArtifactoryId = uuid.NewString() + testProxyUrl = "https://registry-1.docker.io" + testProxyDescription = "docker hub proxy" + testProxyAccessKey = "my-key" + testProxyAccessSecret = "my-secret" ) func fixtureFlagValues(mods ...func(flagValues map[string]string)) map[string]string { @@ -86,6 +89,50 @@ func TestParseInput(t *testing.T) { isValid: true, expectedModel: fixtureInputModel(), }, + { + description: "with description", + argValues: []string{testArtifactoryId}, + flagValues: fixtureFlagValues(func(flagValues map[string]string) { + delete(flagValues, proxyUrlFlag) + flagValues[proxyDescriptionFlag] = testProxyDescription + }), + isValid: true, + expectedModel: fixtureInputModel(func(model *inputModel) { + model.ProxyUrl = nil + model.ProxyDescription = utils.Ptr(testProxyDescription) + }), + }, + { + description: "with credentials", + argValues: []string{testArtifactoryId}, + flagValues: fixtureFlagValues(func(flagValues map[string]string) { + delete(flagValues, proxyUrlFlag) + flagValues[proxyAccessKeyFlag] = testProxyAccessKey + flagValues[proxyAccessSecretFlag] = testProxyAccessSecret + }), + isValid: true, + expectedModel: fixtureInputModel(func(model *inputModel) { + model.ProxyUrl = nil + model.ProxyAccessKey = utils.Ptr(testProxyAccessKey) + model.ProxyAccessSecret = utils.Ptr(testProxyAccessSecret) + }), + }, + { + description: "proxy access key without secret", + argValues: []string{testArtifactoryId}, + flagValues: fixtureFlagValues(func(flagValues map[string]string) { + flagValues[proxyAccessKeyFlag] = testProxyAccessKey + }), + isValid: false, + }, + { + description: "proxy access secret without key", + argValues: []string{testArtifactoryId}, + flagValues: fixtureFlagValues(func(flagValues map[string]string) { + flagValues[proxyAccessSecretFlag] = testProxyAccessSecret + }), + isValid: false, + }, { description: "no flags set", argValues: []string{testArtifactoryId}, @@ -138,6 +185,40 @@ func TestBuildRequest(t *testing.T) { model: fixtureInputModel(), expectedRequest: fixtureRequest(), }, + { + description: "with description", + model: fixtureInputModel(func(model *inputModel) { + model.ProxyUrl = nil + model.ProxyDescription = utils.Ptr(testProxyDescription) + }), + expectedRequest: fixtureRequest(func(request *registry.ApiPatchArtifactoryRequest) { + *request = testClient.DefaultAPI.PatchArtifactory(testCtx, testProjectId, testRegion, testArtifactoryId). + PatchArtifactoryPayload(registry.PatchArtifactoryPayload{ + Proxy: ®istry.UpdateProxyRequest{ + Description: utils.Ptr(testProxyDescription), + }, + }) + }), + }, + { + description: "with credentials", + model: fixtureInputModel(func(model *inputModel) { + model.ProxyUrl = nil + model.ProxyAccessKey = utils.Ptr(testProxyAccessKey) + model.ProxyAccessSecret = utils.Ptr(testProxyAccessSecret) + }), + expectedRequest: fixtureRequest(func(request *registry.ApiPatchArtifactoryRequest) { + *request = testClient.DefaultAPI.PatchArtifactory(testCtx, testProjectId, testRegion, testArtifactoryId). + PatchArtifactoryPayload(registry.PatchArtifactoryPayload{ + Proxy: ®istry.UpdateProxyRequest{ + Credentials: ®istry.Credentials{ + AccessKey: testProxyAccessKey, + AccessSecret: testProxyAccessSecret, + }, + }, + }) + }), + }, } for _, tt := range tests { diff --git a/internal/pkg/services/registry/utils/utils.go b/internal/pkg/services/registry/utils/utils.go index 6e7136074..905569586 100644 --- a/internal/pkg/services/registry/utils/utils.go +++ b/internal/pkg/services/registry/utils/utils.go @@ -12,5 +12,8 @@ func GetArtifactoryName(ctx context.Context, apiClient registry.DefaultAPI, proj if err != nil { return "", fmt.Errorf("get artifactory: %w", err) } + if resp == nil { + return "", fmt.Errorf("get artifactory: empty response") + } return resp.Name, nil } diff --git a/internal/pkg/services/registry/utils/utils_test.go b/internal/pkg/services/registry/utils/utils_test.go new file mode 100644 index 000000000..6f069a727 --- /dev/null +++ b/internal/pkg/services/registry/utils/utils_test.go @@ -0,0 +1,88 @@ +package utils + +import ( + "context" + "fmt" + "testing" + + "github.com/google/uuid" + registry "github.com/stackitcloud/stackit-sdk-go/services/registry/v1api" + + "github.com/stackitcloud/stackit-cli/internal/pkg/utils" +) + +var ( + testProjectId = uuid.NewString() + testArtifactoryId = uuid.NewString() +) + +const ( + testRegion = "eu01" + testArtifactoryName = "my-artifactory" +) + +type registryClientMocked struct { + getArtifactoryFails bool + getArtifactoryResp *registry.Artifactory +} + +func (m *registryClientMocked) newMock() registry.DefaultAPI { + return registry.DefaultAPIServiceMock{ + GetArtifactoryExecuteMock: utils.Ptr(func(_ registry.ApiGetArtifactoryRequest) (*registry.Artifactory, error) { + if m.getArtifactoryFails { + return nil, fmt.Errorf("could not get artifactory") + } + return m.getArtifactoryResp, nil + }), + } +} + +func TestGetArtifactoryName(t *testing.T) { + tests := []struct { + description string + getArtifactoryFails bool + getArtifactoryResp *registry.Artifactory + isValid bool + expectedOutput string + }{ + { + description: "base", + getArtifactoryResp: ®istry.Artifactory{ + Name: testArtifactoryName, + }, + isValid: true, + expectedOutput: testArtifactoryName, + }, + { + description: "get artifactory fails", + getArtifactoryFails: true, + isValid: false, + }, + { + description: "empty response (nil)", + getArtifactoryResp: nil, + isValid: false, + }, + } + + for _, tt := range tests { + t.Run(tt.description, func(t *testing.T) { + client := ®istryClientMocked{ + getArtifactoryFails: tt.getArtifactoryFails, + getArtifactoryResp: tt.getArtifactoryResp, + } + + output, err := GetArtifactoryName(context.Background(), client.newMock(), testProjectId, testRegion, testArtifactoryId) + + if tt.isValid && err != nil { + t.Errorf("failed on valid input: %v", err) + } + if !tt.isValid && err == nil { + t.Errorf("did not fail on invalid input") + } + if tt.isValid && output != tt.expectedOutput { + t.Errorf("expected output %q, got %q", tt.expectedOutput, output) + } + }) + } +}