[SREP-618] feat: Add /config endpoint integration with lazy loading#899
Open
smarthall wants to merge 1 commit intoopenshift:mainfrom
Open
[SREP-618] feat: Add /config endpoint integration with lazy loading#899smarthall wants to merge 1 commit intoopenshift:mainfrom
smarthall wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci
bot
added
the
tide/merge-method-squash
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smarthall The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant Cmd as Refresh Cmd
participant OCM as OCM Interface
participant API as Backplane API Client
participant FS as File System
User->>Cmd: run `ocm-backplane config refresh`
Cmd->>OCM: request access token
OCM-->>Cmd: return token
Cmd->>API: create client (Authorization, User-Agent)
Cmd->>API: FetchRemoteConfig(client)
API-->>Cmd: return RemoteConfig
Cmd->>FS: load local config (viper)
FS-->>Cmd: return local config
Cmd->>Cmd: merge (preserve user, overwrite server-managed)
Cmd->>FS: write updated config file
FS-->>Cmd: write result
Cmd-->>User: report config file path / success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
cmd/ocm-backplane/config/refresh.go (1)
83-86: Silently ignoring allReadInConfigerrors may mask real problems.The
Debugfon line 85 only logs "no existing config file found", but the error could also be a permissions issue, corrupted JSON, etc. Consider checkingos.IsNotExistspecifically and returning other errors.Proposed fix
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") + if os.IsNotExist(err) { + logger.Debugf("No existing config file found, will create new one") + } else { + return fmt.Errorf("failed to read existing config file: %w", err) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ocm-backplane/config/refresh.go` around lines 83 - 86, The current ReadInConfig error is always treated as "no config", which can hide real errors; update the error handling around viper.ReadInConfig() to distinguish a missing config from other failures by checking for viper.ConfigFileNotFoundError (or os.IsNotExist on viper.ConfigFileUsed()) and only log the debug message in that case, otherwise surface/return/log the actual error so permission/parse issues aren't silently ignored; look for the viper.ReadInConfig() call and change its enclosing if-block to handle those two branches accordingly.cmd/ocm-backplane/config/refresh_test.go (1)
43-47: Consider usingGinkgoT()witht.Setenvpattern or ensuringHTTPS_PROXYis set.Line 47 uses
os.Setenvdirectly (with manual cleanup inAfterEach), which is fine for Ginkgo. However, the tests don't setBACKPLANE_URLorHTTPS_PROXYglobally—GetBackplaneConfiguration()will trigger a proxy validation warning. This works because the warning is non-fatal, but it may produce noisy log output during test runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ocm-backplane/config/refresh_test.go` around lines 43 - 47, Replace direct os.Setenv calls in the test setup with GinkgoT().Setenv to avoid manual cleanup and silence proxy warnings: when preparing config in the BeforeEach that calls GetBackplaneConfiguration(), use t := GinkgoT(); t.Setenv("BACKPLANE_CONFIG", configFile) and also set a safe proxy value (e.g., t.Setenv("HTTPS_PROXY", "")) or explicitly set BACKPLANE_URL via t.Setenv to prevent the proxy validation warning; remove the manual os.Unsetenv cleanup in AfterEach when using t.Setenv.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/ocm-backplane/config/refresh.go`:
- Around line 55-63: The User-Agent header value in the BackplaneApi.NewClient
request editor is missing a separator between "backplane-cli" and info.Version;
update the RequestEditors anonymous function that sets
req.Header.Set("User-Agent", ...) to join the name and version with a separator
(e.g., "backplane-cli/"+info.Version or fmt.Sprintf("backplane-cli/%s",
info.Version)) so the header becomes the conventional form like
"backplane-cli/0.1.0".
- Around line 80-121: runRefresh calls GetBackplaneConfiguration which sets
global viper defaults that then get written by viper.WriteConfigAs, leaking
defaults into the user's config; fix by ensuring the read-modify-write uses a
clean Viper instance (or resets globals) — e.g., after calling
GetBackplaneConfiguration in runRefresh, call viper.Reset() before loading the
existing file and updating values, or switch the refresh logic to use a local
viper.New() instance for the load/update/write path (reference runRefresh,
GetBackplaneConfiguration, viper.WriteConfigAs, viper.Reset(), or create/use
viper.New()) so only explicit server-returned or file values are written.
In `@pkg/cli/config/api_client.go`:
- Around line 104-128: Update the incorrect comment that says the API returns
"state name -> transition ID" to instead state that the map contains transition
names (human-readable JIRA state names) in both keys and values; change the
comment near apiConfig.TransitionStates / transitionMap where
JiraTransitionsNamesForAccessRequests is populated and before assigning into
config.ProjectToTransitionsNames (using apiConfig.ProjectKey) to accurately
reflect that values are transition names rather than IDs.
- Around line 34-56: FetchRemoteConfig in DefaultConfigAPIClient leaks the HTTP
response body returned by client.GetConfig; after the nil error check for
client.GetConfig(resp, err) add a deferred close (defer resp.Body.Close()) to
ensure the underlying TCP connection is released, keeping this immediately after
the err check and before calling BackplaneApi.ParseGetConfigResponse so resp is
closed regardless of parsing outcome.
---
Nitpick comments:
In `@cmd/ocm-backplane/config/refresh_test.go`:
- Around line 43-47: Replace direct os.Setenv calls in the test setup with
GinkgoT().Setenv to avoid manual cleanup and silence proxy warnings: when
preparing config in the BeforeEach that calls GetBackplaneConfiguration(), use t
:= GinkgoT(); t.Setenv("BACKPLANE_CONFIG", configFile) and also set a safe proxy
value (e.g., t.Setenv("HTTPS_PROXY", "")) or explicitly set BACKPLANE_URL via
t.Setenv to prevent the proxy validation warning; remove the manual os.Unsetenv
cleanup in AfterEach when using t.Setenv.
In `@cmd/ocm-backplane/config/refresh.go`:
- Around line 83-86: The current ReadInConfig error is always treated as "no
config", which can hide real errors; update the error handling around
viper.ReadInConfig() to distinguish a missing config from other failures by
checking for viper.ConfigFileNotFoundError (or os.IsNotExist on
viper.ConfigFileUsed()) and only log the debug message in that case, otherwise
surface/return/log the actual error so permission/parse issues aren't silently
ignored; look for the viper.ReadInConfig() call and change its enclosing
if-block to handle those two branches accordingly.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
cmd/ocm-backplane/config/config.gocmd/ocm-backplane/config/refresh.gocmd/ocm-backplane/config/refresh_test.gogo.modpkg/cli/config/api_client.gopkg/cli/config/config.gopkg/cli/config/config_test.gopkg/cli/config/mocks/mock_config_api_client.go
openshift-ci
bot
added
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
go.mod (1)
3-3: Consider updating thegodirective to a newer patch or dropping the patch component.Go 1.25.7 (released 2026-02-04) includes security fixes to the
gocommand and thecrypto/tlspackage. Pinning togo 1.25.3means neither CI toolchain enforcement norgo.sumverification mandates those security patches. It's also unconventional — most modules usemajor.minoronly (e.g.,go 1.25) rather than locking to a specific patch.♻️ Suggested update
-go 1.25.3 +go 1.25.7🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 3, Update the go directive in go.mod from "go 1.25.3" to either the latest patched release "go 1.25.7" or the stable minor form "go 1.25"; edit the go directive line in go.mod (the solitary "go" directive) accordingly so CI and go toolchain pick up the security fixes and avoid pinning to a specific patch.pkg/cli/config/api_client_test.go (1)
378-379: Address the deferredFetchRemoteConfigunit test coverage.The comment acknowledges that
FetchRemoteConfigis only tested indirectly and that direct unit tests were deferred pending mock regeneration. Now that the mocks exist (per the PR summary), this technical debt can be resolved in this same PR.Do you want me to generate a direct unit test for
FetchRemoteConfigusing theMockConfigAPIClientalready introduced in this PR?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cli/config/api_client_test.go` around lines 378 - 379, Add a direct unit test that exercises FetchRemoteConfig using the new MockConfigAPIClient: create a test in pkg/cli/config/api_client_test.go that constructs the client under test, injects MockConfigAPIClient with expectations for the /config call (returning a valid config payload and any error cases), calls FetchRemoteConfig, and asserts the returned config and error behavior; reference the FetchRemoteConfig function and MockConfigAPIClient mock to set up expected responses and verify handling of success and failure paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cli/config/api_client_test.go`:
- Around line 207-213: Replace non-fatal map-key checks with fatal checks before
any direct access: use require.Contains(t, result.ProjectToTransitionsNames,
"TEST") (and analogous require.Contains calls for the other keys checked at the
later assertions) so the test stops if the key is missing and we don't proceed
to read result.ProjectToTransitionsNames["TEST"] into transitions; apply this
same change for the repeated patterns at the other spots that directly access
the map before checking existence.
- Around line 28-38: The test dereferences pointer fields on result returned by
mapAPIResponseToRemoteConfig using assert.NotNil, which won't stop execution on
nil and can cause panics; change the nil checks to require.NotNil for result and
each pointer field accessed (e.g., result, result.JiraBaseURL,
result.AssumeInitialArn, result.ProdEnvName, result.JiraConfigForAccessRequests)
before any dereference and keep the existing assert.Equal checks after those
require calls.
---
Nitpick comments:
In `@go.mod`:
- Line 3: Update the go directive in go.mod from "go 1.25.3" to either the
latest patched release "go 1.25.7" or the stable minor form "go 1.25"; edit the
go directive line in go.mod (the solitary "go" directive) accordingly so CI and
go toolchain pick up the security fixes and avoid pinning to a specific patch.
In `@pkg/cli/config/api_client_test.go`:
- Around line 378-379: Add a direct unit test that exercises FetchRemoteConfig
using the new MockConfigAPIClient: create a test in
pkg/cli/config/api_client_test.go that constructs the client under test, injects
MockConfigAPIClient with expectations for the /config call (returning a valid
config payload and any error cases), calls FetchRemoteConfig, and asserts the
returned config and error behavior; reference the FetchRemoteConfig function and
MockConfigAPIClient mock to set up expected responses and verify handling of
success and failure paths.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
go.modpkg/cli/config/api_client_test.go
Member
Author
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/ocm-backplane/config/refresh_test.go (1)
47-64: Prefer restoring the previousBACKPLANE_CONFIGvalue instead of always unsetting it.Line 47 sets process-global env state, and Line 64 always clears it. If the suite started with this variable set, later tests will see altered state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ocm-backplane/config/refresh_test.go` around lines 47 - 64, Before setting BACKPLANE_CONFIG in the test setup, capture its prior state using os.LookupEnv (store value and existence flag) and then in AfterEach restore that prior state: if it existed set it back, otherwise unset it; update the BeforeEach/AfterEach surrounding newRefreshCmd() (where os.Setenv("BACKPLANE_CONFIG", configFile) and os.Unsetenv("BACKPLANE_CONFIG") are present) to use this save-and-restore pattern so global env state is returned to its original value after each test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/ocm-backplane/config/refresh_test.go`:
- Around line 53-68: The AfterEach cleanup currently restores
config.DefaultAPIClient but does not restore the global ocm.DefaultOCMInterface
set to mockOcmInterface in the BeforeEach; update the AfterEach to save the
original ocm.DefaultOCMInterface before overwriting (e.g., originalOCM :=
ocm.DefaultOCMInterface) and then restore it (ocm.DefaultOCMInterface =
originalOCM) alongside restoring config.DefaultAPIClient and other cleanup to
prevent cross-test state leakage.
In `@pkg/cli/config/api_client.go`:
- Around line 115-129: The code may insert an empty transitions entry into
config.ProjectToTransitionsNames when apiConfig.ProjectKey is set but
transitionMap contains only unknown keys; modify the logic in the section that
builds transitions (using transitionMap, transitions, and apiConfig.ProjectKey)
to only write config.ProjectToTransitionsNames[*apiConfig.ProjectKey] =
transitions if at least one transitions field is populated (e.g.,
transitions.OnApproval, transitions.OnCreation, or transitions.OnError is
non-empty) — perform an explicit check for any non-zero field before adding the
map entry to avoid creating empty project transitions.
---
Nitpick comments:
In `@cmd/ocm-backplane/config/refresh_test.go`:
- Around line 47-64: Before setting BACKPLANE_CONFIG in the test setup, capture
its prior state using os.LookupEnv (store value and existence flag) and then in
AfterEach restore that prior state: if it existed set it back, otherwise unset
it; update the BeforeEach/AfterEach surrounding newRefreshCmd() (where
os.Setenv("BACKPLANE_CONFIG", configFile) and os.Unsetenv("BACKPLANE_CONFIG")
are present) to use this save-and-restore pattern so global env state is
returned to its original value after each test.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
cmd/ocm-backplane/config/refresh.gocmd/ocm-backplane/config/refresh_test.gopkg/cli/config/api_client.gopkg/cli/config/api_client_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/ocm-backplane/config/refresh.go
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmd/ocm-backplane/config/refresh_test.go (1)
313-332: Test intent and assertion are misaligned.The spec says it validates a warning, but it only checks success. Either assert warning output or rename the test to reflect success-only behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ocm-backplane/config/refresh_test.go` around lines 313 - 332, The test "should warn if no configuration values are returned" currently only asserts success (Expect(err).To(BeNil())) while its description implies it should assert a warning; either update the assertion to check the logger/output for the expected warning after calling cmd.Execute() (look for the warning produced when FetchRemoteConfig returns an empty remoteConfig) or change the It(...) description to something like "succeeds when no configuration values are returned" to match the current assertion; modify the test body around the cmd.Execute() invocation and the remoteConfig mock accordingly to implement the chosen fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/ocm-backplane/config/refresh_test.go`:
- Around line 269-272: The test "should fail if config cannot be loaded" relies
on BACKPLANE_URL being unset; make the test deterministic by saving the current
BACKPLANE_URL (using os.LookupEnv), unsetting it with
os.Unsetenv("BACKPLANE_URL") at the start of the It block, and restoring the
original value with os.Setenv in a defer so the environment is returned to its
prior state; modify the It(...) block that contains
mockOcmInterface.EXPECT().GetOCMEnvironment().Return(...) to perform this
save/unset/defer restore around the test body.
- Around line 64-71: Move the mock controller finalization to the end of the
AfterEach cleanup so unmet-mock panics don't skip other teardown steps: in the
AfterEach block that currently calls mockCtrl.Finish() first, reorder so that
os.Unsetenv("BACKPLANE_CONFIG"), os.RemoveAll(tempDir), viper.Reset(), and
restoration of config.DefaultAPIClient = originalClient and
ocm.DefaultOCMInterface = originalOCM run before calling mockCtrl.Finish(); keep
the same symbols (mockCtrl.Finish(), os.Unsetenv, os.RemoveAll, viper.Reset,
config.DefaultAPIClient, ocm.DefaultOCMInterface, tempDir, originalClient,
originalOCM) to locate and update the AfterEach cleanup.
In `@pkg/cli/config/api_client.go`:
- Around line 3-10: GetConfig() currently uses context.TODO(), which can block
forever on network stalls; change it to create a cancellable context with a
deadline (e.g., ctx, cancel := context.WithTimeout(context.Background(),
10*time.Second)) in the GetConfig function, replace the context.TODO() call with
ctx, add defer cancel() immediately after creating the context, and ensure the
new ctx is passed into the BackplaneApi client call so the request is bounded by
the timeout; also import time if not already present.
---
Nitpick comments:
In `@cmd/ocm-backplane/config/refresh_test.go`:
- Around line 313-332: The test "should warn if no configuration values are
returned" currently only asserts success (Expect(err).To(BeNil())) while its
description implies it should assert a warning; either update the assertion to
check the logger/output for the expected warning after calling cmd.Execute()
(look for the warning produced when FetchRemoteConfig returns an empty
remoteConfig) or change the It(...) description to something like "succeeds when
no configuration values are returned" to match the current assertion; modify the
test body around the cmd.Execute() invocation and the remoteConfig mock
accordingly to implement the chosen fix.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
cmd/ocm-backplane/config/refresh_test.gopkg/cli/config/api_client.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cmd/ocm-backplane/config/refresh_test.go (1)
83-248: Reduce repeated arrange blocks with a small test helper.
GetOCMEnvironment, token setup, andFetchRemoteConfigmocking are repeated in several specs. A helper would make these tests easier to extend and less error-prone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ocm-backplane/config/refresh_test.go` around lines 83 - 248, Tests repeat the same Arrange steps (calling mockOcmInterface.GetOCMEnvironment, mockOcmInterface.GetOCMAccessToken, and mockAPIClient.FetchRemoteConfig) — introduce a small test helper (e.g., setupMockOCMAndRemoteConfig or prepareMocks) in refresh_test.go that accepts the remoteConfig pointer (and optional token/backplane URL) and performs the repeated gomock expectations for GetOCMEnvironment, GetOCMAccessToken, and FetchRemoteConfig, then replace the duplicated blocks in each It spec with a single call to that helper so each test only declares its specific remoteConfig and assertions.pkg/cli/config/api_client.go (1)
119-127: Consider constants for transition keys.
"approved","in-progress", and"rejected"are repeated magic strings. Extracting constants would reduce typo risk and centralize mapping behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cli/config/api_client.go` around lines 119 - 127, The code uses repeated magic strings "approved", "in-progress", and "rejected" when reading transitionMap; extract these into well-named constants (e.g., const TransitionApproved = "approved", TransitionInProgress = "in-progress", TransitionRejected = "rejected") and replace the literal keys in the map lookups inside the function that accesses transitionMap (references: transitionMap, transitions.OnApproval, transitions.OnCreation, transitions.OnError) so all lookups use the constants; also update any other occurrences of these literals in the same package/file to use the new constants to centralize mapping behavior and avoid typos.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/ocm-backplane/config/refresh_test.go`:
- Around line 322-341: The test "should warn if no configuration values are
returned" currently only checks that cmd.Execute() returns nil; update the test
to assert the expected warning is emitted (or rename the test to match current
behavior). Specifically, capture or mock the logger used by the code under test
(the logger instance the command uses) and add an expectation that a
warning-level message mentioning "no configuration values" (or similar text) is
logged when mockAPIClient.FetchRemoteConfig returns an empty
config.RemoteConfig, then call cmd.Execute() and assert both err == nil and the
warning was logged; alternatively, if you choose to rename, change the It
description to "should succeed when no configuration values are returned" and
keep the existing assertion.
---
Nitpick comments:
In `@cmd/ocm-backplane/config/refresh_test.go`:
- Around line 83-248: Tests repeat the same Arrange steps (calling
mockOcmInterface.GetOCMEnvironment, mockOcmInterface.GetOCMAccessToken, and
mockAPIClient.FetchRemoteConfig) — introduce a small test helper (e.g.,
setupMockOCMAndRemoteConfig or prepareMocks) in refresh_test.go that accepts the
remoteConfig pointer (and optional token/backplane URL) and performs the
repeated gomock expectations for GetOCMEnvironment, GetOCMAccessToken, and
FetchRemoteConfig, then replace the duplicated blocks in each It spec with a
single call to that helper so each test only declares its specific remoteConfig
and assertions.
In `@pkg/cli/config/api_client.go`:
- Around line 119-127: The code uses repeated magic strings "approved",
"in-progress", and "rejected" when reading transitionMap; extract these into
well-named constants (e.g., const TransitionApproved = "approved",
TransitionInProgress = "in-progress", TransitionRejected = "rejected") and
replace the literal keys in the map lookups inside the function that accesses
transitionMap (references: transitionMap, transitions.OnApproval,
transitions.OnCreation, transitions.OnError) so all lookups use the constants;
also update any other occurrences of these literals in the same package/file to
use the new constants to centralize mapping behavior and avoid typos.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
cmd/ocm-backplane/config/refresh_test.gopkg/cli/config/api_client.go
Comment on lines
+322
to
+341
| 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()) | ||
| }) |
There was a problem hiding this comment.
Test intent and assertions are currently misaligned.
The case says “should warn if no configuration values are returned”, but it only asserts cmd.Execute() succeeds. Add a warning assertion (or rename the test) so this behavior is actually validated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/ocm-backplane/config/refresh_test.go` around lines 322 - 341, The test
"should warn if no configuration values are returned" currently only checks that
cmd.Execute() returns nil; update the test to assert the expected warning is
emitted (or rename the test to match current behavior). Specifically, capture or
mock the logger used by the code under test (the logger instance the command
uses) and add an expectation that a warning-level message mentioning "no
configuration values" (or similar text) is logged when
mockAPIClient.FetchRemoteConfig returns an empty config.RemoteConfig, then call
cmd.Execute() and assert both err == nil and the warning was logged;
alternatively, if you choose to rename, change the It description to "should
succeed when no configuration values are returned" and keep the existing
assertion.
Comment on lines
+81
to
+83
| if apiResp.JiraConfigForAccessRequests != nil { | ||
| remote.JiraConfigForAccessRequests = mapJiraAccessRequestConfig(apiResp.JiraConfigForAccessRequests) | ||
| } |
There was a problem hiding this comment.
Distinguish “empty Jira object” from “no Jira remote config.”
If apiResp.JiraConfigForAccessRequests is present but all fields are effectively empty/unmapped, this currently returns a non-nil AccessRequestsJiraConfiguration. That can look like a real remote value and may overwrite existing local values with zero-values.
🔧 Proposed fix
func mapJiraAccessRequestConfig(apiConfig *BackplaneApi.JiraAccessRequestConfig) *AccessRequestsJiraConfiguration {
- config := &AccessRequestsJiraConfiguration{
- ProjectToTransitionsNames: make(map[string]JiraTransitionsNamesForAccessRequests),
- }
+ config := &AccessRequestsJiraConfiguration{}
+ hasValues := false
// Map project-key to both DefaultProject and ProdProject
if apiConfig.ProjectKey != nil {
config.DefaultProject = *apiConfig.ProjectKey
config.ProdProject = *apiConfig.ProjectKey
+ hasValues = true
}
// Map issue-type to both DefaultIssueType and ProdIssueType
if apiConfig.IssueType != nil {
config.DefaultIssueType = *apiConfig.IssueType
config.ProdIssueType = *apiConfig.IssueType
+ hasValues = true
}
@@
// 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 != "") {
+ if config.ProjectToTransitionsNames == nil {
+ config.ProjectToTransitionsNames = make(map[string]JiraTransitionsNamesForAccessRequests)
+ }
config.ProjectToTransitionsNames[*apiConfig.ProjectKey] = transitions
+ hasValues = true
}
}
+ if !hasValues {
+ return nil
+ }
+
return config
}Also applies to: 91-137
Member
Author
|
/retest |
1 similar comment
Member
Author
|
/retest |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #899 +/- ##
==========================================
+ Coverage 53.01% 53.44% +0.42%
==========================================
Files 86 88 +2
Lines 6538 6654 +116
==========================================
+ Hits 3466 3556 +90
- Misses 2610 2633 +23
- Partials 462 465 +3
🚀 New features to boost your workflow:
|
Member
Author
|
/retest |
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 <noreply@anthropic.com>
smarthall
force-pushed
the
feat/config_endpoint
branch
from
732d60c to
6a2ec45
Compare
Member
Author
|
/retest |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
Contributor
|
@smarthall: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
What this PR does / Why we need it?
Implements client-side integration for the new /config API endpoint that allows backplane-cli to fetch server-managed configuration values.
Features:
Implementation:
Server-managed values:
Which Jira/Github issue(s) does this PR fix?
Test coverage checks
Pre-checks (if applicable)
/label tide/merge-method-squash
Summary by CodeRabbit