CONSOLE-5063: Remove AppInitSDK#16019
CONSOLE-5063: Remove AppInitSDK#16019openshift-merge-bot[bot] merged 3 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@logonoff: This pull request references CONSOLE-5063 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
logonoff
force-pushed
the
CONSOLE-5063-s3e7-branch-closing
branch
2 times, most recently
from
c549689 to
a5a76ad
Compare
|
@logonoff: This pull request references CONSOLE-5063 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
component/core
|
/label px-approved |
openshift-ci
bot
added
px-approved
|
@logonoff: This pull request references CONSOLE-5063 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
logonoff
force-pushed
the
CONSOLE-5063-s3e7-branch-closing
branch
from
a5a76ad to
4a8b11d
Compare
frontend/public/module/auth.ts
Outdated
| logoutURL, | ||
| } = window.SERVER_FLAGS; | ||
| // type cast to prevent missing properties errors when building with limited SDK types | ||
| } = (window.SERVER_FLAGS as unknown) as Record<string, string>; |
There was a problem hiding this comment.
I can look into this, maybe it's related to some other type issue.
logonoff
force-pushed
the
CONSOLE-5063-s3e7-branch-closing
branch
2 times, most recently
from
081f4a7 to
5043883
Compare
logonoff
changed the title
logonoff
force-pushed
the
CONSOLE-5063-s3e7-branch-closing
branch
from
66f11df to
39ed3af
Compare
|
@logonoff: This pull request references CONSOLE-5063 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughThis pull request reorganizes the initialization and type infrastructure for OpenShift Console and its dynamic plugin SDK. Type declarations for the 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/console-fetch.ts (1)
83-94:⚠️ Potential issue | 🟠 MajorPotential double application of console headers.
consoleFetchCommoncallsgetConsoleRequestHeaders()and merges them into options (lines 89-94), then passes toconsoleFetchwhich callsapplyConsoleHeadersagain (line 27). This could result in headers being processed twice.Since
consoleFetchis the public API and already handles header application,consoleFetchCommonshould skip the header merge orconsoleFetchshould not apply headers when called fromconsoleFetchCommon.🔧 Proposed fix - remove duplicate header handling in consoleFetchCommon
const consoleFetchCommon = async ( url: string, method: string = 'GET', options: RequestInit = {}, timeout?: number, ): Promise<Response | string> => { - const consoleHeaders = getConsoleRequestHeaders(); - const normalizedConsoleHeaders = normalizeConsoleHeaders(consoleHeaders); - - // Merge headers properly - console headers first, then let options override - const mergedHeaders = { ...normalizedConsoleHeaders, ...options.headers }; - const allOptions = _.defaultsDeep({ method, headers: mergedHeaders }, options); + const allOptions = _.defaultsDeep({ method }, options); const response = await consoleFetch(url, allOptions, timeout);The console headers will be applied by
consoleFetch→applyConsoleHeaders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/console-fetch.ts` around lines 83 - 94, consoleFetchCommon is double-applying console headers: it calls getConsoleRequestHeaders() and merges/normalizes them into mergedHeaders/allOptions while consoleFetch already calls applyConsoleHeaders; remove the header handling from consoleFetchCommon by deleting the getConsoleRequestHeaders(), normalizeConsoleHeaders(), mergedHeaders and related merging logic and pass the original options (or options.headers) through unchanged so consoleFetch can call applyConsoleHeaders once; update any references to allOptions to use options (or a shallow copy) and run tests to confirm header behavior remains correct.
🧹 Nitpick comments (3)
frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/__tests__/console-fetch.spec.ts (1)
78-108: Solid retry test coverage.The retry tests properly verify:
- No retry for 404 (1 call)
- 3 retries for 429 rate limiting
- 3 retries for 409 quota conflicts on POST
Consider adding a test case that verifies successful completion after a transient 429 (mock returns 429 then 200) to ensure the retry loop correctly returns the successful response.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/__tests__/console-fetch.spec.ts` around lines 78 - 108, Add a unit test to console-fetch.spec.ts that simulates a transient 429 followed by a successful 200 to verify the retry loop in consoleFetch returns the successful response: mock window.fetch to first resolve with { status: 429, headers: emptyHeaders } and then resolve with { status: 200, headers: emptyHeaders, json: async () => <payload> }, call await consoleFetch(...) (matching method/args used in other tests), and assert that window.fetch was called the expected number of times and that consoleFetch resolves with the 200 response (or parsed body) instead of throwing; reuse existing symbols like consoleFetch, window.fetch, jest.Mock, emptyHeaders to locate and implement the test.frontend/@types/console/window.d.ts (1)
69-76: Consider stronger typing for debug globals.The
{}type fori18n,store,pluginStore, andwebpackSharedScopeprovides no type safety. While these are debug-only properties, using more specific types (or at leastunknown) would be more idiomatic. That said, this is a minor nit for development-only properties.💡 Optional improvement
- i18n?: {}; // i18next instance, only available in development builds for debugging - store?: {}; // Redux store, only available in development builds for debugging - pluginStore?: {}; // Console plugin store + i18n?: unknown; // i18next instance, only available in development builds for debugging + store?: unknown; // Redux store, only available in development builds for debugging + pluginStore?: unknown; // Console plugin store loadPluginEntry?: Function; // Console plugin entry callback, used to load dynamic plugins - webpackSharedScope?: {}; // webpack shared scope object - Cypress?: {}; - monaco?: {}; + webpackSharedScope?: unknown; // webpack shared scope object + Cypress?: unknown; + monaco?: unknown;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/`@types/console/window.d.ts around lines 69 - 76, The debug global declarations using "{}" (i18n, store, pluginStore, webpackSharedScope) are too weakly typed; change them to at least "unknown" or to more specific types where available (e.g., use the i18next type for i18n, Redux's Store type for store, a PluginStore interface for pluginStore, and the webpack shared scope type for webpackSharedScope) so the declarations in window.d.ts (properties __REDUX_DEVTOOLS_EXTENSION_COMPOSE__, i18n, store, pluginStore, loadPluginEntry, webpackSharedScope, Cypress, monaco) provide meaningful type safety while keeping them optional and dev-only.frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/console-fetch-utils.ts (1)
95-126:applyConsoleHeadersmutates the inputoptionsobject.This function directly modifies
options.headers(lines 99, 110, 113, 116, 122). While functional, mutating input parameters can lead to subtle bugs if the caller reuses the options object. Consider creating a shallow copy of the options and headers.♻️ Proposed immutable approach
export const applyConsoleHeaders = (url: string, options: RequestInit): RequestInit => { const consoleHeaders = getConsoleRequestHeaders(); + const result: RequestInit = { ...options }; + result.headers = { ...(options.headers as Record<string, string>) }; - if (!options.headers) { - options.headers = {}; - } - // Apply console headers, handling array values for multiple headers Object.entries(consoleHeaders || {}).forEach(([key, value]) => { if (Array.isArray(value)) { if (key === 'Impersonate-Group') { - options.headers['X-Console-Impersonate-Groups'] = value.join(','); + result.headers['X-Console-Impersonate-Groups'] = value.join(','); } else { - options.headers[key] = value; + result.headers[key] = value; } } else if (value) { - options.headers[key] = value; + result.headers[key] = value; } }); // X-CSRFToken is used only for non-GET requests targeting bridge - if (options.method === 'GET' || url.indexOf('://') >= 0) { - delete options.headers['X-CSRFToken']; + if (result.method === 'GET' || url.indexOf('://') >= 0) { + delete result.headers['X-CSRFToken']; } - return options; + return result; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/console-fetch-utils.ts` around lines 95 - 126, applyConsoleHeaders currently mutates the input options and its headers; change it to return a new RequestInit object without modifying the original by creating a shallow copy of options and a normalized shallow copy of headers (handle Headers instance, array tuples, or plain object) before applying values from getConsoleRequestHeaders(), preserving existing header values and array handling for 'Impersonate-Group' by writing to 'X-Console-Impersonate-Groups', and perform the X-CSRFToken deletion on the copied headers; update references to options.headers within the function to operate on the copied headers and return the new options object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/`@types/console/window.d.ts:
- Line 25: There are two identical declarations of the quickStarts property in
the Window type declaration; remove the duplicate so the property is declared
only once (keep a single quickStarts: string; entry). Locate the duplicate
quickStarts declaration in the window.d.ts type/interface (the other quickStarts
occurrence) and delete one of them, ensuring the remaining declaration preserves
the exact type (string) and formatting.
In
`@frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/console-fetch.ts`:
- Around line 27-28: The merge using _.defaultsDeep currently uses
_.defaultsDeep({}, initDefaults, op1) which makes initDefaults take precedence
over user options from applyConsoleHeaders (op1); change the merge order so
user-provided options win by calling _.defaultsDeep({}, op1, initDefaults)
(i.e., use op1 then initDefaults) when building allOptions, referencing
applyConsoleHeaders, op1, initDefaults, and allOptions to locate the change.
In `@frontend/public/module/auth.ts`:
- Line 18: The helper isLoginErrorPath currently returns string | boolean
because it uses "path && path === LOGIN_ERROR_PATH"; change it to return a
strict boolean by returning the comparison result directly (e.g., replace the
expression with a boolean result) so callers get a boolean type; update the
isLoginErrorPath function to return the boolean expression (referring to
isLoginErrorPath and LOGIN_ERROR_PATH).
---
Outside diff comments:
In
`@frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/console-fetch.ts`:
- Around line 83-94: consoleFetchCommon is double-applying console headers: it
calls getConsoleRequestHeaders() and merges/normalizes them into
mergedHeaders/allOptions while consoleFetch already calls applyConsoleHeaders;
remove the header handling from consoleFetchCommon by deleting the
getConsoleRequestHeaders(), normalizeConsoleHeaders(), mergedHeaders and related
merging logic and pass the original options (or options.headers) through
unchanged so consoleFetch can call applyConsoleHeaders once; update any
references to allOptions to use options (or a shallow copy) and run tests to
confirm header behavior remains correct.
---
Nitpick comments:
In `@frontend/`@types/console/window.d.ts:
- Around line 69-76: The debug global declarations using "{}" (i18n, store,
pluginStore, webpackSharedScope) are too weakly typed; change them to at least
"unknown" or to more specific types where available (e.g., use the i18next type
for i18n, Redux's Store type for store, a PluginStore interface for pluginStore,
and the webpack shared scope type for webpackSharedScope) so the declarations in
window.d.ts (properties __REDUX_DEVTOOLS_EXTENSION_COMPOSE__, i18n, store,
pluginStore, loadPluginEntry, webpackSharedScope, Cypress, monaco) provide
meaningful type safety while keeping them optional and dev-only.
In
`@frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/__tests__/console-fetch.spec.ts`:
- Around line 78-108: Add a unit test to console-fetch.spec.ts that simulates a
transient 429 followed by a successful 200 to verify the retry loop in
consoleFetch returns the successful response: mock window.fetch to first resolve
with { status: 429, headers: emptyHeaders } and then resolve with { status: 200,
headers: emptyHeaders, json: async () => <payload> }, call await
consoleFetch(...) (matching method/args used in other tests), and assert that
window.fetch was called the expected number of times and that consoleFetch
resolves with the 200 response (or parsed body) instead of throwing; reuse
existing symbols like consoleFetch, window.fetch, jest.Mock, emptyHeaders to
locate and implement the test.
In
`@frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/console-fetch-utils.ts`:
- Around line 95-126: applyConsoleHeaders currently mutates the input options
and its headers; change it to return a new RequestInit object without modifying
the original by creating a shallow copy of options and a normalized shallow copy
of headers (handle Headers instance, array tuples, or plain object) before
applying values from getConsoleRequestHeaders(), preserving existing header
values and array handling for 'Impersonate-Group' by writing to
'X-Console-Impersonate-Groups', and perform the X-CSRFToken deletion on the
copied headers; update references to options.headers within the function to
operate on the copied headers and return the new options object.
| logoutURL: string; | ||
| prometheusBaseURL: string; | ||
| prometheusTenancyBaseURL: string; | ||
| quickStarts: string; |
There was a problem hiding this comment.
Duplicate quickStarts property declaration.
quickStarts: string is declared twice (lines 25 and 39). While TypeScript will accept this (later declaration wins), it's confusing and likely unintentional. Remove one of these declarations.
🔧 Proposed fix
releaseVersion: string;
inactivityTimeout: number;
statuspageID: string;
GOARCH: string;
GOOS: string;
graphqlBaseURL: string;
developerCatalogCategories: string;
perspectives: string;
developerCatalogTypes: string;
userSettingsLocation: string;
addPage: string; // JSON encoded configuration
consolePlugins: string[]; // Console dynamic plugins enabled on the cluster
i18nNamespaces: string[]; // Available i18n namespaces
- quickStarts: string;
projectAccessClusterRoles: string;
controlPlaneTopology: string;Also applies to: 39-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/`@types/console/window.d.ts at line 25, There are two identical
declarations of the quickStarts property in the Window type declaration; remove
the duplicate so the property is declared only once (keep a single quickStarts:
string; entry). Locate the duplicate quickStarts declaration in the window.d.ts
type/interface (the other quickStarts occurrence) and delete one of them,
ensuring the remaining declaration preserves the exact type (string) and
formatting.
| const op1 = applyConsoleHeaders(url, options); | ||
| const allOptions = _.defaultsDeep({}, initDefaults, op1); |
There was a problem hiding this comment.
_.defaultsDeep argument order may cause header override issues.
The current order _.defaultsDeep({}, initDefaults, op1) fills an empty object with initDefaults first, then op1. However, defaultsDeep only fills in properties that are missing or undefined—it doesn't override existing values.
Since initDefaults sets headers: {} and credentials: 'same-origin', these become the base. Then op1 (which contains user options + console headers from applyConsoleHeaders) is applied, but only where initDefaults didn't already set values.
The issue: if initDefaults.credentials is already set, user-provided credentials in options won't override it.
🐛 Proposed fix - reverse the merge order
- const op1 = applyConsoleHeaders(url, options);
- const allOptions = _.defaultsDeep({}, initDefaults, op1);
+ const optionsWithHeaders = applyConsoleHeaders(url, options);
+ const allOptions = _.defaultsDeep({}, optionsWithHeaders, initDefaults);This ensures user-provided options take precedence, with initDefaults filling in only what's missing.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const op1 = applyConsoleHeaders(url, options); | |
| const allOptions = _.defaultsDeep({}, initDefaults, op1); | |
| const optionsWithHeaders = applyConsoleHeaders(url, options); | |
| const allOptions = _.defaultsDeep({}, optionsWithHeaders, initDefaults); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/console-fetch.ts`
around lines 27 - 28, The merge using _.defaultsDeep currently uses
_.defaultsDeep({}, initDefaults, op1) which makes initDefaults take precedence
over user options from applyConsoleHeaders (op1); change the merge order so
user-provided options win by calling _.defaultsDeep({}, op1, initDefaults)
(i.e., use op1 then initDefaults) when building allOptions, referencing
applyConsoleHeaders, op1, initDefaults, and allOptions to locate the change.
| : ''; | ||
|
|
||
| const isLoginErrorPath = (path) => path && path === LOGIN_ERROR_PATH; | ||
| const isLoginErrorPath = (path: string) => path && path === LOGIN_ERROR_PATH; |
There was a problem hiding this comment.
Make isLoginErrorPath return a strict boolean.
path && path === LOGIN_ERROR_PATH yields string | boolean. In TS, this weakens type safety for downstream callers.
Proposed fix
-const isLoginErrorPath = (path: string) => path && path === LOGIN_ERROR_PATH;
+const isLoginErrorPath = (path: string) => !!path && path === LOGIN_ERROR_PATH;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isLoginErrorPath = (path: string) => path && path === LOGIN_ERROR_PATH; | |
| const isLoginErrorPath = (path: string) => !!path && path === LOGIN_ERROR_PATH; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/public/module/auth.ts` at line 18, The helper isLoginErrorPath
currently returns string | boolean because it uses "path && path ===
LOGIN_ERROR_PATH"; change it to return a strict boolean by returning the
comparison result directly (e.g., replace the expression with a boolean result)
so callers get a boolean type; update the isLoginErrorPath function to return
the boolean expression (referring to isLoginErrorPath and LOGIN_ERROR_PATH).
39ed3af to
044a685
Compare
|
/lgtm |
logonoff
force-pushed
the
CONSOLE-5063-s3e7-branch-closing
branch
from
044a685 to
36f32e0
Compare
|
/lgtm |
logonoff
force-pushed
the
CONSOLE-5063-s3e7-branch-closing
branch
from
36f32e0 to
19337cc
Compare
openshift-merge-robot
added
the
needs-rebase
|
/jira refresh |
|
@logonoff: This pull request references CONSOLE-5063 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-merge-robot
removed
the
needs-rebase
frontend/packages/console-dynamic-plugin-sdk/src/utils/fetch/console-fetch.ts
Outdated
Show resolved
Hide resolved
logonoff
force-pushed
the
CONSOLE-5063-s3e7-branch-closing
branch
from
19337cc to
40bb926
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, vojtechszocs 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 |
|
/test analyze backend e2e-gcp-console frontend images okd-scos-images |
|
QE approver: |
|
@logonoff: This pull request references CONSOLE-5063 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/assign @yanpzhan |
|
Launched cluster against the pr, finished regression test on console pages, there was not regression issue。 |
openshift-ci-robot
added
the
verified
|
@yanpzhan: This PR has been marked as verified by DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@logonoff: 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. |
7 participants
useReduxStoreremoved, reverted fallback redux store creation added in enable redux store context in SDK from app host #10448coFetch-related code moved entirely to dynamic-plugin-sdk package to follow up on Convert coFetch utilities to typescript and move to plugin SDK #9660utilsConfigremoved, reverted coFetch sourcing via dependency injection added in enable redux store context in SDK from app host #10448storeHandlerobject directly after initializing our redux storeinitConsolePluginsnow called directly after initializing PluginStore (speeding up loading times slightly)Summary by CodeRabbit
Release Notes