From 21399c78d4bc8e57270f7aed9e0294d7b00e4ed6 Mon Sep 17 00:00:00 2001 From: Daniel Hall Date: Wed, 4 Feb 2026 16:37:55 +1100 Subject: [PATCH 1/6] Add /config endpoint integration with lazy loading Implements client-side integration for the new /config API endpoint that allows backplane-cli to fetch server-managed configuration values. Features: - Refresh command: 'ocm backplane config refresh' to force update server values - Backwards compatible: Existing config files work without changes Implementation: - New ConfigAPIClient interface for fetching remote config - New refresh command to manually update server-managed values Server-managed values: - jira-base-url - assume-initial-arn - prod-env-name - jira-config-for-access-requests Co-Authored-By: Claude Sonnet 4.5 --- cmd/ocm-backplane/config/config.go | 1 + cmd/ocm-backplane/config/refresh.go | 129 ++++++ cmd/ocm-backplane/config/refresh_test.go | 343 ++++++++++++++++ go.mod | 4 +- go.sum | 4 +- pkg/accessrequest/accessRequest_test.go | 9 + pkg/cli/config/api_client.go | 137 +++++++ pkg/cli/config/api_client_test.go | 385 ++++++++++++++++++ pkg/cli/config/config.go | 16 +- pkg/cli/config/config_test.go | 121 ++++++ .../config/mocks/mock_config_api_client.go | 57 +++ pkg/client/mocks/ClientMock.go | 20 + pkg/client/mocks/ClientWithResponsesMock.go | 20 + 13 files changed, 1236 insertions(+), 10 deletions(-) create mode 100644 cmd/ocm-backplane/config/refresh.go create mode 100644 cmd/ocm-backplane/config/refresh_test.go create mode 100644 pkg/cli/config/api_client.go create mode 100644 pkg/cli/config/api_client_test.go create mode 100644 pkg/cli/config/mocks/mock_config_api_client.go diff --git a/cmd/ocm-backplane/config/config.go b/cmd/ocm-backplane/config/config.go index 992fb57e..828cf2b6 100644 --- a/cmd/ocm-backplane/config/config.go +++ b/cmd/ocm-backplane/config/config.go @@ -32,5 +32,6 @@ govcloud Set to true if used in FedRAMP cmd.AddCommand(newGetCmd()) cmd.AddCommand(newSetCmd()) cmd.AddCommand(newTroubleshootCmd()) + cmd.AddCommand(newRefreshCmd()) return cmd } \ No newline at end of file diff --git a/cmd/ocm-backplane/config/refresh.go b/cmd/ocm-backplane/config/refresh.go new file mode 100644 index 00000000..605f99fc --- /dev/null +++ b/cmd/ocm-backplane/config/refresh.go @@ -0,0 +1,129 @@ +package config + +import ( + "context" + "fmt" + "net/http" + + "github.com/spf13/cobra" + "github.com/spf13/viper" + + BackplaneApi "github.com/openshift/backplane-api/pkg/client" + "github.com/openshift/backplane-cli/pkg/cli/config" + "github.com/openshift/backplane-cli/pkg/info" + "github.com/openshift/backplane-cli/pkg/ocm" + logger "github.com/sirupsen/logrus" +) + +func newRefreshCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "refresh", + Short: "Refresh configuration from backplane-api server", + Long: `Fetch the latest configuration values from the backplane-api server and update the local config file. + +This command will overwrite the following server-managed configuration values: + - jira-base-url + - assume-initial-arn + - prod-env-name + - jira-config-for-access-requests + +User-defined values (proxy-url, session-dir, etc.) will not be modified.`, + SilenceUsage: true, + Args: cobra.NoArgs, + RunE: runRefresh, + } + + return cmd +} + +func runRefresh(cmd *cobra.Command, args []string) error { + logger.Info("Refreshing configuration from backplane-api...") + + // Get current config to determine backplane URL + bpConfig, err := config.GetBackplaneConfiguration() + if err != nil { + return fmt.Errorf("failed to load current configuration: %w", err) + } + + // Get OCM access token + token, err := ocm.DefaultOCMInterface.GetOCMAccessToken() + if err != nil { + return fmt.Errorf("failed to get OCM access token: %w", err) + } + + // Create backplane API client + client, err := BackplaneApi.NewClient(bpConfig.URL, func(c *BackplaneApi.Client) error { + c.RequestEditors = append(c.RequestEditors, func(ctx context.Context, req *http.Request) error { + req.Header.Add("Authorization", "Bearer "+*token) + req.Header.Set("User-Agent", "backplane-cli"+info.Version) + req.Header.Set("Backplane-Version", info.DefaultInfoService.GetVersion()) + return nil + }) + return nil + }) + if err != nil { + return fmt.Errorf("failed to create backplane API client: %w", err) + } + + // Fetch fresh config from API + remoteConfig, err := config.DefaultAPIClient.FetchRemoteConfig(client) + if err != nil { + return fmt.Errorf("failed to fetch configuration from server: %w", err) + } + + // Get config file path + configPath, err := config.GetConfigFilePath() + if err != nil { + return fmt.Errorf("failed to get config file path: %w", err) + } + + // Reset viper to clear any defaults that were set by GetBackplaneConfiguration + // This prevents defaults like 'govcloud' from leaking into the user's config file + viper.Reset() + + // Load existing config file to preserve user settings + viper.SetConfigFile(configPath) + viper.SetConfigType("json") + if err := viper.ReadInConfig(); err != nil { + // If config doesn't exist, that's okay - we'll create it + logger.Debugf("No existing config file found, will create new one") + } + + // Update only server-managed values + updated := false + if remoteConfig.JiraBaseURL != nil { + viper.Set(config.JiraBaseURLKey, *remoteConfig.JiraBaseURL) + logger.Infof("Updated jira-base-url: %s", *remoteConfig.JiraBaseURL) + updated = true + } + + if remoteConfig.AssumeInitialArn != nil { + viper.Set(config.AssumeInitialArnKey, *remoteConfig.AssumeInitialArn) + logger.Infof("Updated assume-initial-arn: %s", *remoteConfig.AssumeInitialArn) + updated = true + } + + if remoteConfig.ProdEnvName != nil { + viper.Set(config.ProdEnvNameKey, *remoteConfig.ProdEnvName) + logger.Infof("Updated prod-env-name: %s", *remoteConfig.ProdEnvName) + updated = true + } + + if remoteConfig.JiraConfigForAccessRequests != nil { + viper.Set(config.JiraConfigForAccessRequestsKey, remoteConfig.JiraConfigForAccessRequests) + logger.Info("Updated jira-config-for-access-requests") + updated = true + } + + if !updated { + logger.Warn("No configuration values were returned from the server") + } + + // Write updated config to file + if err := viper.WriteConfigAs(configPath); err != nil { + return fmt.Errorf("failed to write config file: %w", err) + } + + fmt.Printf("Configuration refreshed successfully and saved to %s\n", configPath) + return nil +} diff --git a/cmd/ocm-backplane/config/refresh_test.go b/cmd/ocm-backplane/config/refresh_test.go new file mode 100644 index 00000000..8fb04705 --- /dev/null +++ b/cmd/ocm-backplane/config/refresh_test.go @@ -0,0 +1,343 @@ +package config + +import ( + "fmt" + "os" + "path/filepath" + + "go.uber.org/mock/gomock" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1" + "github.com/spf13/cobra" + "github.com/spf13/viper" + + "github.com/openshift/backplane-cli/pkg/cli/config" + configMocks "github.com/openshift/backplane-cli/pkg/cli/config/mocks" + "github.com/openshift/backplane-cli/pkg/ocm" + ocmMock "github.com/openshift/backplane-cli/pkg/ocm/mocks" +) + +var _ = Describe("refresh command", func() { + var ( + mockCtrl *gomock.Controller + mockOcmInterface *ocmMock.MockOCMInterface + mockAPIClient *configMocks.MockConfigAPIClient + + tempDir string + configFile string + originalClient config.ConfigAPIClient + originalOCM ocm.OCMInterface + cmd *cobra.Command + ) + + BeforeEach(func() { + mockCtrl = gomock.NewController(GinkgoT()) + mockOcmInterface = ocmMock.NewMockOCMInterface(mockCtrl) + mockAPIClient = configMocks.NewMockConfigAPIClient(mockCtrl) + + // Save original API client and OCM interface + originalClient = config.DefaultAPIClient + originalOCM = ocm.DefaultOCMInterface + + // Create temp directory for config file + var err error + tempDir, err = os.MkdirTemp("", "refresh-test-*") + Expect(err).To(BeNil()) + + configFile = filepath.Join(tempDir, "config.json") + os.Setenv("BACKPLANE_CONFIG", configFile) + + // Reset viper + viper.Reset() + + // Set up OCM mock + ocm.DefaultOCMInterface = mockOcmInterface + + // Set up config API client mock + config.DefaultAPIClient = mockAPIClient + + // Create the refresh command + cmd = newRefreshCmd() + }) + + AfterEach(func() { + os.Unsetenv("BACKPLANE_CONFIG") + os.RemoveAll(tempDir) + viper.Reset() + config.DefaultAPIClient = originalClient + ocm.DefaultOCMInterface = originalOCM + mockCtrl.Finish() + }) + + Context("when refreshing configuration successfully", func() { + It("should fetch remote config and save to file", func() { + // Set up existing config file with user settings + existingConfig := `{ + "proxy-url": ["http://user-proxy:3128"], + "session-dir": "/custom/session" + }` + err := os.WriteFile(configFile, []byte(existingConfig), 0600) + Expect(err).To(BeNil()) + + // Set up backplane URL + ocmEnv, _ := cmv1.NewEnvironment().BackplaneURL("https://backplane.example.com").Build() + mockOcmInterface.EXPECT().GetOCMEnvironment().Return(ocmEnv, nil).AnyTimes() + + // Mock OCM token + token := "test-token-12345" + mockOcmInterface.EXPECT().GetOCMAccessToken().Return(&token, nil) + + // Mock remote config response + jiraBaseURL := "https://jira.example.com" + assumeArn := "arn:aws:iam::123456789:role/Test-Role" + prodEnv := "production-test" + + remoteConfig := &config.RemoteConfig{ + JiraBaseURL: &jiraBaseURL, + AssumeInitialArn: &assumeArn, + ProdEnvName: &prodEnv, + JiraConfigForAccessRequests: &config.AccessRequestsJiraConfiguration{ + DefaultProject: "TEST", + DefaultIssueType: "Story", + ProdProject: "PROD", + ProdIssueType: "Incident", + ProjectToTransitionsNames: map[string]config.JiraTransitionsNamesForAccessRequests{ + "TEST": { + OnCreation: "In Progress", + OnApproval: "Done", + OnError: "Closed", + }, + }, + }, + } + + mockAPIClient.EXPECT(). + FetchRemoteConfig(gomock.Any()). + Return(remoteConfig, nil) + + // Execute command + err = cmd.Execute() + Expect(err).To(BeNil()) + + // Verify config file was updated + fileContent, err := os.ReadFile(configFile) + Expect(err).To(BeNil()) + + // Verify server-managed values were written + Expect(string(fileContent)).To(ContainSubstring("https://jira.example.com")) + Expect(string(fileContent)).To(ContainSubstring("arn:aws:iam::123456789:role/Test-Role")) + Expect(string(fileContent)).To(ContainSubstring("production-test")) + Expect(string(fileContent)).To(ContainSubstring("TEST")) + + // Verify user settings were preserved + Expect(string(fileContent)).To(ContainSubstring("http://user-proxy:3128")) + Expect(string(fileContent)).To(ContainSubstring("/custom/session")) + }) + + It("should create config file if it doesn't exist", func() { + // No existing config file + + // Set up backplane URL + ocmEnv, _ := cmv1.NewEnvironment().BackplaneURL("https://backplane.example.com").Build() + mockOcmInterface.EXPECT().GetOCMEnvironment().Return(ocmEnv, nil).AnyTimes() + + // Mock OCM token + token := "test-token-12345" + mockOcmInterface.EXPECT().GetOCMAccessToken().Return(&token, nil) + + // Mock remote config response + jiraBaseURL := "https://jira.example.com" + remoteConfig := &config.RemoteConfig{ + JiraBaseURL: &jiraBaseURL, + } + + mockAPIClient.EXPECT(). + FetchRemoteConfig(gomock.Any()). + Return(remoteConfig, nil) + + // Execute command + err := cmd.Execute() + Expect(err).To(BeNil()) + + // Verify config file was created + _, err = os.Stat(configFile) + Expect(err).To(BeNil()) + }) + + It("should overwrite existing server-managed values", func() { + // Set up existing config file with old server values + existingConfig := `{ + "jira-base-url": "https://old-jira.example.com", + "assume-initial-arn": "arn:aws:iam::111111111:role/Old-Role", + "prod-env-name": "old-production", + "proxy-url": ["http://user-proxy:3128"] + }` + err := os.WriteFile(configFile, []byte(existingConfig), 0600) + Expect(err).To(BeNil()) + + // Set up backplane URL + ocmEnv, _ := cmv1.NewEnvironment().BackplaneURL("https://backplane.example.com").Build() + mockOcmInterface.EXPECT().GetOCMEnvironment().Return(ocmEnv, nil).AnyTimes() + + // Mock OCM token + token := "test-token-12345" + mockOcmInterface.EXPECT().GetOCMAccessToken().Return(&token, nil) + + // Mock remote config response with NEW values + newJiraBaseURL := "https://new-jira.example.com" + newAssumeArn := "arn:aws:iam::999999999:role/New-Role" + newProdEnv := "new-production" + + remoteConfig := &config.RemoteConfig{ + JiraBaseURL: &newJiraBaseURL, + AssumeInitialArn: &newAssumeArn, + ProdEnvName: &newProdEnv, + } + + mockAPIClient.EXPECT(). + FetchRemoteConfig(gomock.Any()). + Return(remoteConfig, nil) + + // Execute command + err = cmd.Execute() + Expect(err).To(BeNil()) + + // Verify config file has NEW values + fileContent, err := os.ReadFile(configFile) + Expect(err).To(BeNil()) + + Expect(string(fileContent)).To(ContainSubstring("new-jira.example.com")) + Expect(string(fileContent)).To(ContainSubstring("arn:aws:iam::999999999:role/New-Role")) + Expect(string(fileContent)).To(ContainSubstring("new-production")) + + // Verify OLD values are not present + Expect(string(fileContent)).NotTo(ContainSubstring("old-jira.example.com")) + Expect(string(fileContent)).NotTo(ContainSubstring("arn:aws:iam::111111111:role/Old-Role")) + Expect(string(fileContent)).NotTo(ContainSubstring("old-production")) + + // Verify user settings were preserved + Expect(string(fileContent)).To(ContainSubstring("http://user-proxy:3128")) + }) + + It("should not leak viper defaults into the config file", func() { + // Set up existing config file with minimal user settings (no govcloud) + existingConfig := `{ + "proxy-url": ["http://user-proxy:3128"] + }` + err := os.WriteFile(configFile, []byte(existingConfig), 0600) + Expect(err).To(BeNil()) + + // Set up backplane URL + ocmEnv, _ := cmv1.NewEnvironment().BackplaneURL("https://backplane.example.com").Build() + mockOcmInterface.EXPECT().GetOCMEnvironment().Return(ocmEnv, nil).AnyTimes() + + // Mock OCM token + token := "test-token-12345" + mockOcmInterface.EXPECT().GetOCMAccessToken().Return(&token, nil) + + // Mock remote config response + jiraBaseURL := "https://jira.example.com" + remoteConfig := &config.RemoteConfig{ + JiraBaseURL: &jiraBaseURL, + } + + mockAPIClient.EXPECT(). + FetchRemoteConfig(gomock.Any()). + Return(remoteConfig, nil) + + // Execute command + err = cmd.Execute() + Expect(err).To(BeNil()) + + // Read the written config file + fileContent, err := os.ReadFile(configFile) + Expect(err).To(BeNil()) + + // Verify server-managed value was written + Expect(string(fileContent)).To(ContainSubstring("https://jira.example.com")) + + // Verify user setting was preserved + Expect(string(fileContent)).To(ContainSubstring("http://user-proxy:3128")) + + // CRITICAL: Verify that viper defaults (like govcloud) did NOT leak into the file + Expect(string(fileContent)).NotTo(ContainSubstring("govcloud")) + }) + }) + + Context("when errors occur", func() { + It("should fail if config cannot be loaded", func() { + // Save and unset BACKPLANE_URL to ensure config load fails + originalBackplaneURL, wasSet := os.LookupEnv("BACKPLANE_URL") + os.Unsetenv("BACKPLANE_URL") + defer func() { + if wasSet { + os.Setenv("BACKPLANE_URL", originalBackplaneURL) + } + }() + + // Don't set BACKPLANE_URL env var, and no OCM env + mockOcmInterface.EXPECT().GetOCMEnvironment().Return(nil, fmt.Errorf("no OCM environment")) + + // Execute command + err := cmd.Execute() + Expect(err).NotTo(BeNil()) + Expect(err.Error()).To(ContainSubstring("failed to load current configuration")) + }) + + It("should fail if OCM access token cannot be obtained", func() { + // Set up backplane URL + ocmEnv, _ := cmv1.NewEnvironment().BackplaneURL("https://backplane.example.com").Build() + mockOcmInterface.EXPECT().GetOCMEnvironment().Return(ocmEnv, nil).AnyTimes() + + // Mock OCM token failure + mockOcmInterface.EXPECT().GetOCMAccessToken().Return(nil, fmt.Errorf("token error")) + + // Execute command + err := cmd.Execute() + Expect(err).NotTo(BeNil()) + Expect(err.Error()).To(ContainSubstring("failed to get OCM access token")) + }) + + It("should fail if remote config fetch fails", func() { + // Set up backplane URL + ocmEnv, _ := cmv1.NewEnvironment().BackplaneURL("https://backplane.example.com").Build() + mockOcmInterface.EXPECT().GetOCMEnvironment().Return(ocmEnv, nil).AnyTimes() + + // Mock OCM token + token := "test-token-12345" + mockOcmInterface.EXPECT().GetOCMAccessToken().Return(&token, nil) + + // Mock remote config fetch failure + mockAPIClient.EXPECT(). + FetchRemoteConfig(gomock.Any()). + Return(nil, fmt.Errorf("API error")) + + // Execute command + err := cmd.Execute() + Expect(err).NotTo(BeNil()) + Expect(err.Error()).To(ContainSubstring("failed to fetch configuration from server")) + }) + + It("should warn if no configuration values are returned", func() { + // Set up backplane URL + ocmEnv, _ := cmv1.NewEnvironment().BackplaneURL("https://backplane.example.com").Build() + mockOcmInterface.EXPECT().GetOCMEnvironment().Return(ocmEnv, nil).AnyTimes() + + // Mock OCM token + token := "test-token-12345" + mockOcmInterface.EXPECT().GetOCMAccessToken().Return(&token, nil) + + // Mock remote config response with no values + remoteConfig := &config.RemoteConfig{} + + mockAPIClient.EXPECT(). + FetchRemoteConfig(gomock.Any()). + Return(remoteConfig, nil) + + // Execute command - should still succeed but log warning + err := cmd.Execute() + Expect(err).To(BeNil()) + }) + }) +}) diff --git a/go.mod b/go.mod index de1c5d50..beb844ee 100644 --- a/go.mod +++ b/go.mod @@ -19,13 +19,14 @@ require ( github.com/openshift-online/ocm-api-model/clientapi v0.0.449 github.com/openshift-online/ocm-cli v1.0.10 github.com/openshift-online/ocm-sdk-go v0.1.494 - github.com/openshift/backplane-api v0.0.0-20251117160932-490f3091533f + github.com/openshift/backplane-api v0.0.0-20260205054653-459856398d59 github.com/openshift/client-go v0.0.0-20221019143426-16aed247da5c github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c github.com/sirupsen/logrus v1.9.4 github.com/spf13/cobra v1.10.1 github.com/spf13/pflag v1.0.10 github.com/spf13/viper v1.21.0 + github.com/stretchr/testify v1.11.1 github.com/trivago/tgo v1.0.7 go.uber.org/mock v0.6.0 golang.org/x/term v0.40.0 @@ -50,6 +51,7 @@ require ( github.com/oasdiff/yaml3 v0.0.0-20250309153720-d2182401db90 // indirect github.com/openshift-online/ocm-api-model/model v0.0.449 // indirect github.com/openshift-online/ocm-common v0.0.29 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect github.com/segmentio/asm v1.1.3 // indirect github.com/segmentio/encoding v0.5.3 // indirect github.com/yosida95/uritemplate/v3 v3.0.2 // indirect diff --git a/go.sum b/go.sum index 7f97a9aa..690e2964 100644 --- a/go.sum +++ b/go.sum @@ -403,8 +403,8 @@ github.com/openshift-online/ocm-sdk-go v0.1.494 h1:bsXWWaF6tKKD9xztX0frzINtqrFnR github.com/openshift-online/ocm-sdk-go v0.1.494/go.mod h1:p8GJutgkXGbJJdNj5GFWzQTA41cROD9PL0W5L45tqHU= github.com/openshift/api v0.0.0-20221018124113-7edcfe3c76cb h1:QsBjYe5UfHIZi/3SMzQBIjYDKnWqZxq50eQkBp9eUew= github.com/openshift/api v0.0.0-20221018124113-7edcfe3c76cb/go.mod h1:JRz+ZvTqu9u7t6suhhPTacbFl5K65Y6rJbNM7HjWA3g= -github.com/openshift/backplane-api v0.0.0-20251117160932-490f3091533f h1:7VGTnBRgyKpyXOitwNVtR05HsPNZARqow8rU5Qo2bGY= -github.com/openshift/backplane-api v0.0.0-20251117160932-490f3091533f/go.mod h1:0+HQ/Ujo/hRKpBFePq2Zitrk6sc5viJNrDtbBTx1uh0= +github.com/openshift/backplane-api v0.0.0-20260205054653-459856398d59 h1:p7AeWpCHmIWm+WHh8R+AOjKveS6NGbwhTpNdZpMws6g= +github.com/openshift/backplane-api v0.0.0-20260205054653-459856398d59/go.mod h1:0+HQ/Ujo/hRKpBFePq2Zitrk6sc5viJNrDtbBTx1uh0= github.com/openshift/client-go v0.0.0-20221019143426-16aed247da5c h1:CV76yFOTXmq9VciBR3Bve5ZWzSxdft7gaMVB3kS0rwg= github.com/openshift/client-go v0.0.0-20221019143426-16aed247da5c/go.mod h1:lFMO8mLHXWFzSdYvGNo8ivF9SfF6zInA8ZGw4phRnUE= github.com/pelletier/go-toml/v2 v2.2.4 h1:mye9XuhQ6gvn5h28+VilKrrPoQVanw5PMw/TB0t5Ec4= diff --git a/pkg/accessrequest/accessRequest_test.go b/pkg/accessrequest/accessRequest_test.go index 3c410b26..30e56224 100644 --- a/pkg/accessrequest/accessRequest_test.go +++ b/pkg/accessrequest/accessRequest_test.go @@ -210,6 +210,15 @@ var _ = Describe(testDesc, func() { Context("issue project is known from the config", func() { It("should succeed even if the JIRA token is not defined in the config as the issue existence cannot be disproved", func() { + // Unset JIRA_API_TOKEN environment variable to ensure no token is picked up + originalToken := os.Getenv("JIRA_API_TOKEN") + os.Unsetenv("JIRA_API_TOKEN") + DeferCleanup(func() { + if originalToken != "" { + os.Setenv("JIRA_API_TOKEN", originalToken) + } + }) + mockOcmInterface.EXPECT().CreateClusterAccessRequest(nil, clusterID, reason, issueID, durationStr).Return(accessRequest, nil).Times(1) returnedAccessRequest, err := CreateAccessRequest(nil, clusterID, reason, issueID, duration) diff --git a/pkg/cli/config/api_client.go b/pkg/cli/config/api_client.go new file mode 100644 index 00000000..add41e09 --- /dev/null +++ b/pkg/cli/config/api_client.go @@ -0,0 +1,137 @@ +package config + +import ( + "context" + "fmt" + "net/http" + "time" + + BackplaneApi "github.com/openshift/backplane-api/pkg/client" + logger "github.com/sirupsen/logrus" +) + +//go:generate mockgen -destination=mocks/mock_config_api_client.go -package=mocks github.com/openshift/backplane-cli/pkg/cli/config ConfigAPIClient + +// ConfigAPIClient handles fetching configuration from backplane-api +type ConfigAPIClient interface { + FetchRemoteConfig(client BackplaneApi.ClientInterface) (*RemoteConfig, error) +} + +// DefaultConfigAPIClient implements ConfigAPIClient using the real API +type DefaultConfigAPIClient struct{} + +// DefaultAPIClient is the default instance used for API calls +var DefaultAPIClient ConfigAPIClient = &DefaultConfigAPIClient{} + +// RemoteConfig represents the config values fetched from /config endpoint +type RemoteConfig struct { + JiraBaseURL *string + AssumeInitialArn *string + ProdEnvName *string + JiraConfigForAccessRequests *AccessRequestsJiraConfiguration +} + +// FetchRemoteConfig calls the /config endpoint and parses the response +func (c *DefaultConfigAPIClient) FetchRemoteConfig(client BackplaneApi.ClientInterface) (*RemoteConfig, error) { + logger.Debugf("Fetching remote configuration from /backplane/config") + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + resp, err := client.GetConfig(ctx) + if err != nil { + return nil, fmt.Errorf("failed to call /config endpoint: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("unexpected status code from /config endpoint: %d", resp.StatusCode) + } + + configResp, err := BackplaneApi.ParseGetConfigResponse(resp) + if err != nil { + return nil, fmt.Errorf("failed to parse /config response: %w", err) + } + + if configResp.JSON200 == nil { + return nil, fmt.Errorf("empty response from /config endpoint") + } + + return mapAPIResponseToRemoteConfig(configResp.JSON200), nil +} + +// mapAPIResponseToRemoteConfig converts API response to internal RemoteConfig +func mapAPIResponseToRemoteConfig(apiResp *BackplaneApi.ClientConfig) *RemoteConfig { + remote := &RemoteConfig{} + + // Map simple string fields + if apiResp.JiraBaseUrl != nil { + remote.JiraBaseURL = apiResp.JiraBaseUrl + } + + if apiResp.AssumeInitialArn != nil { + remote.AssumeInitialArn = apiResp.AssumeInitialArn + } + + if apiResp.ProdEnvName != nil { + remote.ProdEnvName = apiResp.ProdEnvName + } + + // Map Jira config if present + if apiResp.JiraConfigForAccessRequests != nil { + remote.JiraConfigForAccessRequests = mapJiraAccessRequestConfig(apiResp.JiraConfigForAccessRequests) + } + + return remote +} + +// mapJiraAccessRequestConfig converts API Jira config to internal format +// The API schema is simpler (single project/issue-type) than the CLI structure +// (default/prod project pairs), so we map the API values to both default and prod +func mapJiraAccessRequestConfig(apiConfig *BackplaneApi.JiraAccessRequestConfig) *AccessRequestsJiraConfiguration { + config := &AccessRequestsJiraConfiguration{ + ProjectToTransitionsNames: make(map[string]JiraTransitionsNamesForAccessRequests), + } + + // Map project-key to both DefaultProject and ProdProject + if apiConfig.ProjectKey != nil { + config.DefaultProject = *apiConfig.ProjectKey + config.ProdProject = *apiConfig.ProjectKey + } + + // Map issue-type to both DefaultIssueType and ProdIssueType + if apiConfig.IssueType != nil { + config.DefaultIssueType = *apiConfig.IssueType + config.ProdIssueType = *apiConfig.IssueType + } + + // Map transition-states if present + if apiConfig.TransitionStates != nil && len(*apiConfig.TransitionStates) > 0 { + // Extract transition state names from the map + // The API has a map[string]string containing transition names (human-readable JIRA state names) in both keys and values + // We need to populate ProjectToTransitionsNames with transition names + + transitionMap := *apiConfig.TransitionStates + + // Try to extract known transition states + transitions := JiraTransitionsNamesForAccessRequests{} + + // Look for common transition state names in the map keys + if val, ok := transitionMap["approved"]; ok { + transitions.OnApproval = val + } + if val, ok := transitionMap["in-progress"]; ok { + transitions.OnCreation = val + } + if val, ok := transitionMap["rejected"]; ok { + transitions.OnError = val + } + + // Add the transitions for the project key only if at least one field is populated + if apiConfig.ProjectKey != nil && (transitions.OnApproval != "" || transitions.OnCreation != "" || transitions.OnError != "") { + config.ProjectToTransitionsNames[*apiConfig.ProjectKey] = transitions + } + } + + return config +} diff --git a/pkg/cli/config/api_client_test.go b/pkg/cli/config/api_client_test.go new file mode 100644 index 00000000..34e38d71 --- /dev/null +++ b/pkg/cli/config/api_client_test.go @@ -0,0 +1,385 @@ +package config + +import ( + "testing" + + BackplaneApi "github.com/openshift/backplane-api/pkg/client" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMapAPIResponseToRemoteConfig(t *testing.T) { + t.Run("maps all fields when present", func(t *testing.T) { + jiraURL := "https://jira.example.com" + arnValue := "arn:aws:iam::123456789:role/Test-Role" + prodEnv := "production" + projectKey := "TESTPROJ" + issueType := "Story" + + input := &BackplaneApi.ClientConfig{ + JiraBaseUrl: &jiraURL, + AssumeInitialArn: &arnValue, + ProdEnvName: &prodEnv, + JiraConfigForAccessRequests: &BackplaneApi.JiraAccessRequestConfig{ + ProjectKey: &projectKey, + IssueType: &issueType, + }, + } + + result := mapAPIResponseToRemoteConfig(input) + + require.NotNil(t, result) + require.NotNil(t, result.JiraBaseURL) + assert.Equal(t, jiraURL, *result.JiraBaseURL) + require.NotNil(t, result.AssumeInitialArn) + assert.Equal(t, arnValue, *result.AssumeInitialArn) + require.NotNil(t, result.ProdEnvName) + assert.Equal(t, prodEnv, *result.ProdEnvName) + require.NotNil(t, result.JiraConfigForAccessRequests) + assert.Equal(t, projectKey, result.JiraConfigForAccessRequests.DefaultProject) + }) + + t.Run("handles nil JiraBaseUrl", func(t *testing.T) { + input := &BackplaneApi.ClientConfig{ + JiraBaseUrl: nil, + } + + result := mapAPIResponseToRemoteConfig(input) + + assert.NotNil(t, result) + assert.Nil(t, result.JiraBaseURL) + }) + + t.Run("handles nil AssumeInitialArn", func(t *testing.T) { + input := &BackplaneApi.ClientConfig{ + AssumeInitialArn: nil, + } + + result := mapAPIResponseToRemoteConfig(input) + + assert.NotNil(t, result) + assert.Nil(t, result.AssumeInitialArn) + }) + + t.Run("handles nil ProdEnvName", func(t *testing.T) { + input := &BackplaneApi.ClientConfig{ + ProdEnvName: nil, + } + + result := mapAPIResponseToRemoteConfig(input) + + assert.NotNil(t, result) + assert.Nil(t, result.ProdEnvName) + }) + + t.Run("handles nil JiraConfigForAccessRequests", func(t *testing.T) { + input := &BackplaneApi.ClientConfig{ + JiraConfigForAccessRequests: nil, + } + + result := mapAPIResponseToRemoteConfig(input) + + assert.NotNil(t, result) + assert.Nil(t, result.JiraConfigForAccessRequests) + }) + + t.Run("handles completely empty config", func(t *testing.T) { + input := &BackplaneApi.ClientConfig{} + + result := mapAPIResponseToRemoteConfig(input) + + assert.NotNil(t, result) + assert.Nil(t, result.JiraBaseURL) + assert.Nil(t, result.AssumeInitialArn) + assert.Nil(t, result.ProdEnvName) + assert.Nil(t, result.JiraConfigForAccessRequests) + }) + + t.Run("calls mapJiraAccessRequestConfig when Jira config is present", func(t *testing.T) { + projectKey := "TEST" + issueType := "Bug" + transitionStates := map[string]string{ + "approved": "Done", + } + + input := &BackplaneApi.ClientConfig{ + JiraConfigForAccessRequests: &BackplaneApi.JiraAccessRequestConfig{ + ProjectKey: &projectKey, + IssueType: &issueType, + TransitionStates: &transitionStates, + }, + } + + result := mapAPIResponseToRemoteConfig(input) + + assert.NotNil(t, result) + assert.NotNil(t, result.JiraConfigForAccessRequests) + assert.Equal(t, "TEST", result.JiraConfigForAccessRequests.DefaultProject) + assert.Equal(t, "Bug", result.JiraConfigForAccessRequests.DefaultIssueType) + }) +} + +func TestMapJiraAccessRequestConfig(t *testing.T) { + t.Run("maps project key to both default and prod", func(t *testing.T) { + projectKey := "TESTPROJECT" + + input := &BackplaneApi.JiraAccessRequestConfig{ + ProjectKey: &projectKey, + } + + result := mapJiraAccessRequestConfig(input) + + assert.NotNil(t, result) + assert.Equal(t, "TESTPROJECT", result.DefaultProject) + assert.Equal(t, "TESTPROJECT", result.ProdProject) + assert.NotNil(t, result.ProjectToTransitionsNames) + }) + + t.Run("maps issue type to both default and prod", func(t *testing.T) { + issueType := "Story" + + input := &BackplaneApi.JiraAccessRequestConfig{ + IssueType: &issueType, + } + + result := mapJiraAccessRequestConfig(input) + + assert.NotNil(t, result) + assert.Equal(t, "Story", result.DefaultIssueType) + assert.Equal(t, "Story", result.ProdIssueType) + }) + + t.Run("maps all basic fields together", func(t *testing.T) { + projectKey := "MYPROJECT" + issueType := "Task" + + input := &BackplaneApi.JiraAccessRequestConfig{ + ProjectKey: &projectKey, + IssueType: &issueType, + } + + result := mapJiraAccessRequestConfig(input) + + assert.Equal(t, "MYPROJECT", result.DefaultProject) + assert.Equal(t, "MYPROJECT", result.ProdProject) + assert.Equal(t, "Task", result.DefaultIssueType) + assert.Equal(t, "Task", result.ProdIssueType) + }) + + t.Run("handles nil project key", func(t *testing.T) { + input := &BackplaneApi.JiraAccessRequestConfig{ + ProjectKey: nil, + } + + result := mapJiraAccessRequestConfig(input) + + assert.NotNil(t, result) + assert.Empty(t, result.DefaultProject) + assert.Empty(t, result.ProdProject) + }) + + t.Run("handles nil issue type", func(t *testing.T) { + input := &BackplaneApi.JiraAccessRequestConfig{ + IssueType: nil, + } + + result := mapJiraAccessRequestConfig(input) + + assert.NotNil(t, result) + assert.Empty(t, result.DefaultIssueType) + assert.Empty(t, result.ProdIssueType) + }) + + t.Run("maps all transition states correctly", func(t *testing.T) { + projectKey := "TEST" + transitionStates := map[string]string{ + "approved": "Done", + "in-progress": "In Progress", + "rejected": "Closed", + } + + input := &BackplaneApi.JiraAccessRequestConfig{ + ProjectKey: &projectKey, + TransitionStates: &transitionStates, + } + + result := mapJiraAccessRequestConfig(input) + + assert.NotNil(t, result) + require.Contains(t, result.ProjectToTransitionsNames, "TEST") + + transitions := result.ProjectToTransitionsNames["TEST"] + assert.Equal(t, "Done", transitions.OnApproval) + assert.Equal(t, "In Progress", transitions.OnCreation) + assert.Equal(t, "Closed", transitions.OnError) + }) + + t.Run("handles partial transition states - only approved", func(t *testing.T) { + projectKey := "PARTIAL" + transitionStates := map[string]string{ + "approved": "Done", + } + + input := &BackplaneApi.JiraAccessRequestConfig{ + ProjectKey: &projectKey, + TransitionStates: &transitionStates, + } + + result := mapJiraAccessRequestConfig(input) + + require.Contains(t, result.ProjectToTransitionsNames, "PARTIAL") + transitions := result.ProjectToTransitionsNames["PARTIAL"] + assert.Equal(t, "Done", transitions.OnApproval) + assert.Empty(t, transitions.OnCreation) + assert.Empty(t, transitions.OnError) + }) + + t.Run("handles partial transition states - only in-progress", func(t *testing.T) { + projectKey := "PARTIAL2" + transitionStates := map[string]string{ + "in-progress": "Working", + } + + input := &BackplaneApi.JiraAccessRequestConfig{ + ProjectKey: &projectKey, + TransitionStates: &transitionStates, + } + + result := mapJiraAccessRequestConfig(input) + + require.Contains(t, result.ProjectToTransitionsNames, "PARTIAL2") + transitions := result.ProjectToTransitionsNames["PARTIAL2"] + assert.Empty(t, transitions.OnApproval) + assert.Equal(t, "Working", transitions.OnCreation) + assert.Empty(t, transitions.OnError) + }) + + t.Run("handles partial transition states - only rejected", func(t *testing.T) { + projectKey := "PARTIAL3" + transitionStates := map[string]string{ + "rejected": "Failed", + } + + input := &BackplaneApi.JiraAccessRequestConfig{ + ProjectKey: &projectKey, + TransitionStates: &transitionStates, + } + + result := mapJiraAccessRequestConfig(input) + + require.Contains(t, result.ProjectToTransitionsNames, "PARTIAL3") + transitions := result.ProjectToTransitionsNames["PARTIAL3"] + assert.Empty(t, transitions.OnApproval) + assert.Empty(t, transitions.OnCreation) + assert.Equal(t, "Failed", transitions.OnError) + }) + + t.Run("ignores unknown transition state keys", func(t *testing.T) { + projectKey := "UNKNOWN" + transitionStates := map[string]string{ + "unknown-key": "SomeValue", + "another-key": "AnotherValue", + "approved": "Done", + } + + input := &BackplaneApi.JiraAccessRequestConfig{ + ProjectKey: &projectKey, + TransitionStates: &transitionStates, + } + + result := mapJiraAccessRequestConfig(input) + + require.Contains(t, result.ProjectToTransitionsNames, "UNKNOWN") + transitions := result.ProjectToTransitionsNames["UNKNOWN"] + assert.Equal(t, "Done", transitions.OnApproval) + assert.Empty(t, transitions.OnCreation) + assert.Empty(t, transitions.OnError) + }) + + t.Run("handles nil transition states", func(t *testing.T) { + projectKey := "NOTRANS" + + input := &BackplaneApi.JiraAccessRequestConfig{ + ProjectKey: &projectKey, + TransitionStates: nil, + } + + result := mapJiraAccessRequestConfig(input) + + assert.NotNil(t, result) + assert.NotContains(t, result.ProjectToTransitionsNames, "NOTRANS") + }) + + t.Run("handles empty transition states map", func(t *testing.T) { + projectKey := "EMPTY" + transitionStates := map[string]string{} + + input := &BackplaneApi.JiraAccessRequestConfig{ + ProjectKey: &projectKey, + TransitionStates: &transitionStates, + } + + result := mapJiraAccessRequestConfig(input) + + assert.NotNil(t, result) + assert.NotContains(t, result.ProjectToTransitionsNames, "EMPTY") + }) + + t.Run("does not add to ProjectToTransitionsNames if project key is nil", func(t *testing.T) { + transitionStates := map[string]string{ + "approved": "Done", + } + + input := &BackplaneApi.JiraAccessRequestConfig{ + ProjectKey: nil, + TransitionStates: &transitionStates, + } + + result := mapJiraAccessRequestConfig(input) + + assert.NotNil(t, result) + assert.Len(t, result.ProjectToTransitionsNames, 0) + }) + + t.Run("complete config with all fields", func(t *testing.T) { + projectKey := "COMPLETE" + issueType := "Epic" + transitionStates := map[string]string{ + "approved": "Completed", + "in-progress": "In Development", + "rejected": "Cancelled", + } + + input := &BackplaneApi.JiraAccessRequestConfig{ + ProjectKey: &projectKey, + IssueType: &issueType, + TransitionStates: &transitionStates, + } + + result := mapJiraAccessRequestConfig(input) + + assert.Equal(t, "COMPLETE", result.DefaultProject) + assert.Equal(t, "COMPLETE", result.ProdProject) + assert.Equal(t, "Epic", result.DefaultIssueType) + assert.Equal(t, "Epic", result.ProdIssueType) + + require.Contains(t, result.ProjectToTransitionsNames, "COMPLETE") + transitions := result.ProjectToTransitionsNames["COMPLETE"] + assert.Equal(t, "Completed", transitions.OnApproval) + assert.Equal(t, "In Development", transitions.OnCreation) + assert.Equal(t, "Cancelled", transitions.OnError) + }) + + t.Run("empty config initializes map", func(t *testing.T) { + input := &BackplaneApi.JiraAccessRequestConfig{} + + result := mapJiraAccessRequestConfig(input) + + assert.NotNil(t, result) + assert.NotNil(t, result.ProjectToTransitionsNames) + assert.Len(t, result.ProjectToTransitionsNames, 0) + }) +} + +// Note: FetchRemoteConfig is tested indirectly through the refresh_test.go integration tests +// Direct unit testing would require regenerating mocks after the /config endpoint was added diff --git a/pkg/cli/config/config.go b/pkg/cli/config/config.go index b6d0c565..a006ce14 100644 --- a/pkg/cli/config/config.go +++ b/pkg/cli/config/config.go @@ -54,8 +54,9 @@ type BackplaneConfiguration struct { } const ( - prodEnvNameKey = "prod-env-name" - jiraBaseURLKey = "jira-base-url" + ProdEnvNameKey = "prod-env-name" + JiraBaseURLKey = "jira-base-url" + AssumeInitialArnKey = "assume-initial-arn" JiraTokenViperKey = "jira-token" JiraConfigForAccessRequestsKey = "jira-config-for-access-requests" prodEnvNameDefaultValue = "production" @@ -102,6 +103,7 @@ func GetConfigFilePath() (string, error) { return configFilePath, nil } + // GetBackplaneConfiguration parses and returns the given backplane configuration func GetBackplaneConfiguration() (bpConfig BackplaneConfiguration, err error) { return GetBackplaneConfigurationWithConn(nil) @@ -110,8 +112,8 @@ func GetBackplaneConfiguration() (bpConfig BackplaneConfiguration, err error) { // GetBackplaneConfiguration parses and returns the given backplane configuration using attributes // from provided OCM connection func GetBackplaneConfigurationWithConn(ocmConn *ocmsdk.Connection) (bpConfig BackplaneConfiguration, err error) { - viper.SetDefault(prodEnvNameKey, prodEnvNameDefaultValue) - viper.SetDefault(jiraBaseURLKey, JiraBaseURLDefaultValue) + viper.SetDefault(ProdEnvNameKey, prodEnvNameDefaultValue) + viper.SetDefault(JiraBaseURLKey, JiraBaseURLDefaultValue) viper.SetDefault(JiraConfigForAccessRequestsKey, JiraConfigForAccessRequestsDefaultValue) viper.SetDefault(GovcloudDefaultValueKey, GovcloudDefaultValue) filePath, err := GetConfigFilePath() @@ -201,7 +203,7 @@ func GetBackplaneConfigurationWithConn(ocmConn *ocmsdk.Connection) (bpConfig Bac } bpConfig.SessionDirectory = viper.GetString("session-dir") - bpConfig.AssumeInitialArn = viper.GetString("assume-initial-arn") + bpConfig.AssumeInitialArn = viper.GetString(AssumeInitialArnKey) bpConfig.DisplayClusterInfo = viper.GetBool("display-cluster-info") bpConfig.DisableKubePS1Warning = viper.GetBool("disable-kube-ps1-warning") @@ -218,10 +220,10 @@ func GetBackplaneConfigurationWithConn(ocmConn *ocmsdk.Connection) (bpConfig Bac } // OCM prod env name is optional as there is a default value - bpConfig.ProdEnvName = viper.GetString(prodEnvNameKey) + bpConfig.ProdEnvName = viper.GetString(ProdEnvNameKey) // JIRA base URL is optional as there is a default value - bpConfig.JiraBaseURL = viper.GetString(jiraBaseURLKey) + bpConfig.JiraBaseURL = viper.GetString(JiraBaseURLKey) // JIRA token is optional // JIRA_API_TOKEN env var takes precedence, fallback to config file diff --git a/pkg/cli/config/config_test.go b/pkg/cli/config/config_test.go index 00d6016e..eb01f117 100644 --- a/pkg/cli/config/config_test.go +++ b/pkg/cli/config/config_test.go @@ -232,3 +232,124 @@ func TestValidateConfig(t *testing.T) { }) } } + +// TestNoAutomaticConfigFetch verifies that GetBackplaneConfiguration +// does NOT automatically fetch config from the API when values are missing +func TestNoAutomaticConfigFetch(t *testing.T) { + t.Run("uses default values when config file is missing", func(t *testing.T) { + viper.Reset() + + svr := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // This should NOT be called - if it is, the test will fail + t.Error("HTTP request was made - automatic config fetch detected!") + w.WriteHeader(http.StatusInternalServerError) + })) + defer svr.Close() + + userDefinedProxy := "example-proxy" + t.Setenv("BACKPLANE_URL", svr.URL) + t.Setenv("HTTPS_PROXY", userDefinedProxy) + + config, err := GetBackplaneConfiguration() + if err != nil { + t.Error(err) + } + + // Should use default values, not fetch from API + if config.JiraBaseURL != JiraBaseURLDefaultValue { + t.Errorf("expected default JiraBaseURL %s, got %s", JiraBaseURLDefaultValue, config.JiraBaseURL) + } + if config.ProdEnvName != "production" { + t.Errorf("expected default ProdEnvName 'production', got %s", config.ProdEnvName) + } + }) + + t.Run("uses config file values without fetching from API", func(t *testing.T) { + viper.Reset() + + // Create a temp config file with specific values + tmpDir := t.TempDir() + configPath := tmpDir + "/config.json" + configContent := `{ + "jira-base-url": "https://custom-jira.example.com", + "assume-initial-arn": "arn:aws:iam::999999999:role/Custom-Role", + "prod-env-name": "custom-prod" + }` + err := os.WriteFile(configPath, []byte(configContent), 0600) + if err != nil { + t.Fatal(err) + } + + t.Setenv("BACKPLANE_CONFIG", configPath) + + svr := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // This should NOT be called + t.Error("HTTP request was made - automatic config fetch detected!") + w.WriteHeader(http.StatusInternalServerError) + })) + defer svr.Close() + + userDefinedProxy := "example-proxy" + t.Setenv("BACKPLANE_URL", svr.URL) + t.Setenv("HTTPS_PROXY", userDefinedProxy) + + config, err := GetBackplaneConfiguration() + if err != nil { + t.Error(err) + } + + // Should use values from config file + if config.JiraBaseURL != "https://custom-jira.example.com" { + t.Errorf("expected JiraBaseURL from config file, got %s", config.JiraBaseURL) + } + if config.AssumeInitialArn != "arn:aws:iam::999999999:role/Custom-Role" { + t.Errorf("expected AssumeInitialArn from config file, got %s", config.AssumeInitialArn) + } + if config.ProdEnvName != "custom-prod" { + t.Errorf("expected ProdEnvName from config file, got %s", config.ProdEnvName) + } + }) + + t.Run("partial config file uses defaults for missing values without API fetch", func(t *testing.T) { + viper.Reset() + + // Create a config file with only some values + tmpDir := t.TempDir() + configPath := tmpDir + "/config.json" + configContent := `{ + "jira-base-url": "https://partial-jira.example.com" + }` + err := os.WriteFile(configPath, []byte(configContent), 0600) + if err != nil { + t.Fatal(err) + } + + t.Setenv("BACKPLANE_CONFIG", configPath) + + svr := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // This should NOT be called + t.Error("HTTP request was made - automatic config fetch detected!") + w.WriteHeader(http.StatusInternalServerError) + })) + defer svr.Close() + + userDefinedProxy := "example-proxy" + t.Setenv("BACKPLANE_URL", svr.URL) + t.Setenv("HTTPS_PROXY", userDefinedProxy) + + config, err := GetBackplaneConfiguration() + if err != nil { + t.Error(err) + } + + // Should use value from config file + if config.JiraBaseURL != "https://partial-jira.example.com" { + t.Errorf("expected JiraBaseURL from config file, got %s", config.JiraBaseURL) + } + + // Should use defaults for missing values, NOT fetch from API + if config.ProdEnvName != "production" { + t.Errorf("expected default ProdEnvName 'production', got %s", config.ProdEnvName) + } + }) +} diff --git a/pkg/cli/config/mocks/mock_config_api_client.go b/pkg/cli/config/mocks/mock_config_api_client.go new file mode 100644 index 00000000..15e5a995 --- /dev/null +++ b/pkg/cli/config/mocks/mock_config_api_client.go @@ -0,0 +1,57 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/openshift/backplane-cli/pkg/cli/config (interfaces: ConfigAPIClient) +// +// Generated by this command: +// +// mockgen -destination=mocks/mock_config_api_client.go -package=mocks github.com/openshift/backplane-cli/pkg/cli/config ConfigAPIClient +// + +// Package mocks is a generated GoMock package. +package mocks + +import ( + reflect "reflect" + + Openapi "github.com/openshift/backplane-api/pkg/client" + config "github.com/openshift/backplane-cli/pkg/cli/config" + gomock "go.uber.org/mock/gomock" +) + +// MockConfigAPIClient is a mock of ConfigAPIClient interface. +type MockConfigAPIClient struct { + ctrl *gomock.Controller + recorder *MockConfigAPIClientMockRecorder + isgomock struct{} +} + +// MockConfigAPIClientMockRecorder is the mock recorder for MockConfigAPIClient. +type MockConfigAPIClientMockRecorder struct { + mock *MockConfigAPIClient +} + +// NewMockConfigAPIClient creates a new mock instance. +func NewMockConfigAPIClient(ctrl *gomock.Controller) *MockConfigAPIClient { + mock := &MockConfigAPIClient{ctrl: ctrl} + mock.recorder = &MockConfigAPIClientMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockConfigAPIClient) EXPECT() *MockConfigAPIClientMockRecorder { + return m.recorder +} + +// FetchRemoteConfig mocks base method. +func (m *MockConfigAPIClient) FetchRemoteConfig(client Openapi.ClientInterface) (*config.RemoteConfig, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "FetchRemoteConfig", client) + ret0, _ := ret[0].(*config.RemoteConfig) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// FetchRemoteConfig indicates an expected call of FetchRemoteConfig. +func (mr *MockConfigAPIClientMockRecorder) FetchRemoteConfig(client any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FetchRemoteConfig", reflect.TypeOf((*MockConfigAPIClient)(nil).FetchRemoteConfig), client) +} diff --git a/pkg/client/mocks/ClientMock.go b/pkg/client/mocks/ClientMock.go index 3b6c3350..c407f991 100644 --- a/pkg/client/mocks/ClientMock.go +++ b/pkg/client/mocks/ClientMock.go @@ -403,6 +403,26 @@ func (mr *MockClientInterfaceMockRecorder) GetCloudCredentials(ctx, clusterId an return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetCloudCredentials", reflect.TypeOf((*MockClientInterface)(nil).GetCloudCredentials), varargs...) } +// GetConfig mocks base method. +func (m *MockClientInterface) GetConfig(ctx context.Context, reqEditors ...Openapi.RequestEditorFn) (*http.Response, error) { + m.ctrl.T.Helper() + varargs := []any{ctx} + for _, a := range reqEditors { + varargs = append(varargs, a) + } + ret := m.ctrl.Call(m, "GetConfig", varargs...) + ret0, _ := ret[0].(*http.Response) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetConfig indicates an expected call of GetConfig. +func (mr *MockClientInterfaceMockRecorder) GetConfig(ctx any, reqEditors ...any) *gomock.Call { + mr.mock.ctrl.T.Helper() + varargs := append([]any{ctx}, reqEditors...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetConfig", reflect.TypeOf((*MockClientInterface)(nil).GetConfig), varargs...) +} + // GetJobLogs mocks base method. func (m *MockClientInterface) GetJobLogs(ctx context.Context, clusterId, jobId string, params *Openapi.GetJobLogsParams, reqEditors ...Openapi.RequestEditorFn) (*http.Response, error) { m.ctrl.T.Helper() diff --git a/pkg/client/mocks/ClientWithResponsesMock.go b/pkg/client/mocks/ClientWithResponsesMock.go index 22ab9765..102121fe 100644 --- a/pkg/client/mocks/ClientWithResponsesMock.go +++ b/pkg/client/mocks/ClientWithResponsesMock.go @@ -402,6 +402,26 @@ func (mr *MockClientWithResponsesInterfaceMockRecorder) GetCloudCredentialsWithR return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetCloudCredentialsWithResponse", reflect.TypeOf((*MockClientWithResponsesInterface)(nil).GetCloudCredentialsWithResponse), varargs...) } +// GetConfigWithResponse mocks base method. +func (m *MockClientWithResponsesInterface) GetConfigWithResponse(ctx context.Context, reqEditors ...Openapi.RequestEditorFn) (*Openapi.GetConfigResponse, error) { + m.ctrl.T.Helper() + varargs := []any{ctx} + for _, a := range reqEditors { + varargs = append(varargs, a) + } + ret := m.ctrl.Call(m, "GetConfigWithResponse", varargs...) + ret0, _ := ret[0].(*Openapi.GetConfigResponse) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetConfigWithResponse indicates an expected call of GetConfigWithResponse. +func (mr *MockClientWithResponsesInterfaceMockRecorder) GetConfigWithResponse(ctx any, reqEditors ...any) *gomock.Call { + mr.mock.ctrl.T.Helper() + varargs := append([]any{ctx}, reqEditors...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetConfigWithResponse", reflect.TypeOf((*MockClientWithResponsesInterface)(nil).GetConfigWithResponse), varargs...) +} + // GetJobLogsWithResponse mocks base method. func (m *MockClientWithResponsesInterface) GetJobLogsWithResponse(ctx context.Context, clusterId, jobId string, params *Openapi.GetJobLogsParams, reqEditors ...Openapi.RequestEditorFn) (*Openapi.GetJobLogsResponse, error) { m.ctrl.T.Helper() From d082814fba8714505c589ba1a0223e968f083105 Mon Sep 17 00:00:00 2001 From: Daniel Hall Date: Fri, 27 Feb 2026 10:47:28 +1100 Subject: [PATCH 2/6] Verify the logging Signed-off-by: Daniel Hall --- cmd/ocm-backplane/config/refresh_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/cmd/ocm-backplane/config/refresh_test.go b/cmd/ocm-backplane/config/refresh_test.go index 8fb04705..7010de30 100644 --- a/cmd/ocm-backplane/config/refresh_test.go +++ b/cmd/ocm-backplane/config/refresh_test.go @@ -1,6 +1,7 @@ package config import ( + "bytes" "fmt" "os" "path/filepath" @@ -9,6 +10,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1" + logger "github.com/sirupsen/logrus" "github.com/spf13/cobra" "github.com/spf13/viper" @@ -320,6 +322,14 @@ var _ = Describe("refresh command", func() { }) It("should warn if no configuration values are returned", func() { + // Capture logger output + var logBuffer bytes.Buffer + originalOutput := logger.StandardLogger().Out + logger.SetOutput(&logBuffer) + DeferCleanup(func() { + logger.SetOutput(originalOutput) + }) + // Set up backplane URL ocmEnv, _ := cmv1.NewEnvironment().BackplaneURL("https://backplane.example.com").Build() mockOcmInterface.EXPECT().GetOCMEnvironment().Return(ocmEnv, nil).AnyTimes() @@ -338,6 +348,10 @@ var _ = Describe("refresh command", func() { // Execute command - should still succeed but log warning err := cmd.Execute() Expect(err).To(BeNil()) + + // Verify warning was logged + logOutput := logBuffer.String() + Expect(logOutput).To(ContainSubstring("No configuration values were returned from the server")) }) }) }) From 4625868eecc7f9d2c479323227944e64ca83ae8f Mon Sep 17 00:00:00 2001 From: Daniel Hall Date: Fri, 27 Feb 2026 10:55:09 +1100 Subject: [PATCH 3/6] Don't write empty jira values to config Signed-off-by: Daniel Hall --- pkg/cli/config/api_client.go | 11 +++++++++++ pkg/cli/config/api_client_test.go | 23 ++++++++--------------- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/pkg/cli/config/api_client.go b/pkg/cli/config/api_client.go index add41e09..45289535 100644 --- a/pkg/cli/config/api_client.go +++ b/pkg/cli/config/api_client.go @@ -88,21 +88,26 @@ func mapAPIResponseToRemoteConfig(apiResp *BackplaneApi.ClientConfig) *RemoteCon // mapJiraAccessRequestConfig converts API Jira config to internal format // The API schema is simpler (single project/issue-type) than the CLI structure // (default/prod project pairs), so we map the API values to both default and prod +// Returns nil if the API config is effectively empty to avoid overwriting local config with zero-values func mapJiraAccessRequestConfig(apiConfig *BackplaneApi.JiraAccessRequestConfig) *AccessRequestsJiraConfiguration { config := &AccessRequestsJiraConfiguration{ ProjectToTransitionsNames: make(map[string]JiraTransitionsNamesForAccessRequests), } + hasData := false + // Map project-key to both DefaultProject and ProdProject if apiConfig.ProjectKey != nil { config.DefaultProject = *apiConfig.ProjectKey config.ProdProject = *apiConfig.ProjectKey + hasData = true } // Map issue-type to both DefaultIssueType and ProdIssueType if apiConfig.IssueType != nil { config.DefaultIssueType = *apiConfig.IssueType config.ProdIssueType = *apiConfig.IssueType + hasData = true } // Map transition-states if present @@ -130,8 +135,14 @@ func mapJiraAccessRequestConfig(apiConfig *BackplaneApi.JiraAccessRequestConfig) // Add the transitions for the project key only if at least one field is populated if apiConfig.ProjectKey != nil && (transitions.OnApproval != "" || transitions.OnCreation != "" || transitions.OnError != "") { config.ProjectToTransitionsNames[*apiConfig.ProjectKey] = transitions + hasData = true } } + // Return nil if no meaningful data was set to avoid overwriting existing local config + if !hasData { + return nil + } + return config } diff --git a/pkg/cli/config/api_client_test.go b/pkg/cli/config/api_client_test.go index 34e38d71..b84894ac 100644 --- a/pkg/cli/config/api_client_test.go +++ b/pkg/cli/config/api_client_test.go @@ -166,28 +166,24 @@ func TestMapJiraAccessRequestConfig(t *testing.T) { assert.Equal(t, "Task", result.ProdIssueType) }) - t.Run("handles nil project key", func(t *testing.T) { + t.Run("handles nil project key - returns nil for empty config", func(t *testing.T) { input := &BackplaneApi.JiraAccessRequestConfig{ ProjectKey: nil, } result := mapJiraAccessRequestConfig(input) - assert.NotNil(t, result) - assert.Empty(t, result.DefaultProject) - assert.Empty(t, result.ProdProject) + assert.Nil(t, result, "should return nil when config has no meaningful data") }) - t.Run("handles nil issue type", func(t *testing.T) { + t.Run("handles nil issue type - returns nil for empty config", func(t *testing.T) { input := &BackplaneApi.JiraAccessRequestConfig{ IssueType: nil, } result := mapJiraAccessRequestConfig(input) - assert.NotNil(t, result) - assert.Empty(t, result.DefaultIssueType) - assert.Empty(t, result.ProdIssueType) + assert.Nil(t, result, "should return nil when config has no meaningful data") }) t.Run("maps all transition states correctly", func(t *testing.T) { @@ -325,7 +321,7 @@ func TestMapJiraAccessRequestConfig(t *testing.T) { assert.NotContains(t, result.ProjectToTransitionsNames, "EMPTY") }) - t.Run("does not add to ProjectToTransitionsNames if project key is nil", func(t *testing.T) { + t.Run("does not add to ProjectToTransitionsNames if project key is nil - returns nil", func(t *testing.T) { transitionStates := map[string]string{ "approved": "Done", } @@ -337,8 +333,7 @@ func TestMapJiraAccessRequestConfig(t *testing.T) { result := mapJiraAccessRequestConfig(input) - assert.NotNil(t, result) - assert.Len(t, result.ProjectToTransitionsNames, 0) + assert.Nil(t, result, "should return nil when transition states exist but project key is nil (no meaningful data)") }) t.Run("complete config with all fields", func(t *testing.T) { @@ -370,14 +365,12 @@ func TestMapJiraAccessRequestConfig(t *testing.T) { assert.Equal(t, "Cancelled", transitions.OnError) }) - t.Run("empty config initializes map", func(t *testing.T) { + t.Run("empty config returns nil to avoid overwriting local config", func(t *testing.T) { input := &BackplaneApi.JiraAccessRequestConfig{} result := mapJiraAccessRequestConfig(input) - assert.NotNil(t, result) - assert.NotNil(t, result.ProjectToTransitionsNames) - assert.Len(t, result.ProjectToTransitionsNames, 0) + assert.Nil(t, result, "should return nil for completely empty config to prevent overwriting existing local values with zero-values") }) } From d373c9380b055247a723adbc396f6342ed27b976 Mon Sep 17 00:00:00 2001 From: Daniel Hall Date: Mon, 2 Mar 2026 09:41:50 +1100 Subject: [PATCH 4/6] Fix proxy support for config refresh command Use backplaneapi.DefaultClientUtils.GetBackplaneClient() instead of direct client creation to ensure proper proxy configuration and authentication. This fixes timeout issues when running behind corporate proxies. Signed-off-by: Daniel Hall Co-Authored-By: Claude Sonnet 4.5 --- cmd/ocm-backplane/config/refresh.go | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/cmd/ocm-backplane/config/refresh.go b/cmd/ocm-backplane/config/refresh.go index 605f99fc..e6827260 100644 --- a/cmd/ocm-backplane/config/refresh.go +++ b/cmd/ocm-backplane/config/refresh.go @@ -1,16 +1,13 @@ package config import ( - "context" "fmt" - "net/http" "github.com/spf13/cobra" "github.com/spf13/viper" - BackplaneApi "github.com/openshift/backplane-api/pkg/client" + "github.com/openshift/backplane-cli/pkg/backplaneapi" "github.com/openshift/backplane-cli/pkg/cli/config" - "github.com/openshift/backplane-cli/pkg/info" "github.com/openshift/backplane-cli/pkg/ocm" logger "github.com/sirupsen/logrus" ) @@ -51,16 +48,8 @@ func runRefresh(cmd *cobra.Command, args []string) error { return fmt.Errorf("failed to get OCM access token: %w", err) } - // Create backplane API client - client, err := BackplaneApi.NewClient(bpConfig.URL, func(c *BackplaneApi.Client) error { - c.RequestEditors = append(c.RequestEditors, func(ctx context.Context, req *http.Request) error { - req.Header.Add("Authorization", "Bearer "+*token) - req.Header.Set("User-Agent", "backplane-cli"+info.Version) - req.Header.Set("Backplane-Version", info.DefaultInfoService.GetVersion()) - return nil - }) - return nil - }) + // Create backplane API client with proper proxy support + client, err := backplaneapi.DefaultClientUtils.GetBackplaneClient(bpConfig.URL, *token, bpConfig.ProxyURL) if err != nil { return fmt.Errorf("failed to create backplane API client: %w", err) } From 9290f8e4c970e85c3edd55ba679e87d4bca3d576 Mon Sep 17 00:00:00 2001 From: Daniel Hall Date: Mon, 2 Mar 2026 09:55:19 +1100 Subject: [PATCH 5/6] Improve code quality in mapJiraAccessRequestConfig - Add early return to avoid wasteful allocation when config is empty - Extract magic strings to constants (transitionStateApproved, etc.) - Enhance documentation to clarify nil-return contract - Reuse hasTransitions variable to avoid duplicate checks These changes improve efficiency by preventing unnecessary struct and map allocations in the common case where the API returns empty config. Signed-off-by: Daniel Hall Co-Authored-By: Claude Sonnet 4.5 --- pkg/cli/config/api_client.go | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/pkg/cli/config/api_client.go b/pkg/cli/config/api_client.go index 45289535..87279bda 100644 --- a/pkg/cli/config/api_client.go +++ b/pkg/cli/config/api_client.go @@ -10,6 +10,13 @@ import ( logger "github.com/sirupsen/logrus" ) +// Transition state keys used in the API response +const ( + transitionStateApproved = "approved" + transitionStateInProgress = "in-progress" + transitionStateRejected = "rejected" +) + //go:generate mockgen -destination=mocks/mock_config_api_client.go -package=mocks github.com/openshift/backplane-cli/pkg/cli/config ConfigAPIClient // ConfigAPIClient handles fetching configuration from backplane-api @@ -85,11 +92,20 @@ func mapAPIResponseToRemoteConfig(apiResp *BackplaneApi.ClientConfig) *RemoteCon return remote } -// mapJiraAccessRequestConfig converts API Jira config to internal format +// mapJiraAccessRequestConfig converts API Jira config to internal format. // The API schema is simpler (single project/issue-type) than the CLI structure -// (default/prod project pairs), so we map the API values to both default and prod -// Returns nil if the API config is effectively empty to avoid overwriting local config with zero-values +// (default/prod project pairs), so we map the API values to both default and prod. +// +// Returns nil if the API config contains no meaningful data to avoid overwriting +// local config with zero-values. This nil-return contract is relied upon by the caller +// to preserve existing local configuration when the API returns empty values. func mapJiraAccessRequestConfig(apiConfig *BackplaneApi.JiraAccessRequestConfig) *AccessRequestsJiraConfiguration { + // Early return if completely empty - avoids wasteful allocation + hasTransitions := apiConfig.TransitionStates != nil && len(*apiConfig.TransitionStates) > 0 + if apiConfig.ProjectKey == nil && apiConfig.IssueType == nil && !hasTransitions { + return nil + } + config := &AccessRequestsJiraConfiguration{ ProjectToTransitionsNames: make(map[string]JiraTransitionsNamesForAccessRequests), } @@ -111,7 +127,7 @@ func mapJiraAccessRequestConfig(apiConfig *BackplaneApi.JiraAccessRequestConfig) } // Map transition-states if present - if apiConfig.TransitionStates != nil && len(*apiConfig.TransitionStates) > 0 { + if hasTransitions { // Extract transition state names from the map // The API has a map[string]string containing transition names (human-readable JIRA state names) in both keys and values // We need to populate ProjectToTransitionsNames with transition names @@ -122,13 +138,13 @@ func mapJiraAccessRequestConfig(apiConfig *BackplaneApi.JiraAccessRequestConfig) transitions := JiraTransitionsNamesForAccessRequests{} // Look for common transition state names in the map keys - if val, ok := transitionMap["approved"]; ok { + if val, ok := transitionMap[transitionStateApproved]; ok { transitions.OnApproval = val } - if val, ok := transitionMap["in-progress"]; ok { + if val, ok := transitionMap[transitionStateInProgress]; ok { transitions.OnCreation = val } - if val, ok := transitionMap["rejected"]; ok { + if val, ok := transitionMap[transitionStateRejected]; ok { transitions.OnError = val } From aac0f4f6d7bd99908ee2eac4df968e2cc0f1f23c Mon Sep 17 00:00:00 2001 From: Daniel Hall Date: Mon, 2 Mar 2026 10:18:36 +1100 Subject: [PATCH 6/6] Fix test isolation for TestNoAutomaticConfigFetch Set BACKPLANE_CONFIG to a non-existent path in the test to prevent it from reading the user's actual config file. This ensures test isolation and prevents test failures when the user's config contains different values than the test expects. Signed-off-by: Daniel Hall Co-Authored-By: Claude Sonnet 4.5 --- pkg/cli/config/config_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/cli/config/config_test.go b/pkg/cli/config/config_test.go index eb01f117..58d31888 100644 --- a/pkg/cli/config/config_test.go +++ b/pkg/cli/config/config_test.go @@ -246,6 +246,10 @@ func TestNoAutomaticConfigFetch(t *testing.T) { })) defer svr.Close() + // Point to a non-existent config file to ensure test isolation + tmpDir := t.TempDir() + t.Setenv("BACKPLANE_CONFIG", tmpDir+"/nonexistent-config.json") + userDefinedProxy := "example-proxy" t.Setenv("BACKPLANE_URL", svr.URL) t.Setenv("HTTPS_PROXY", userDefinedProxy)