OCPBUGS-78980: Update @graphql-codegen packages for Node.js 25 compatibility#16240
OCPBUGS-78980: Update @graphql-codegen packages for Node.js 25 compatibility#16240stefanonardo wants to merge 1 commit into
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@stefanonardo: This pull request references Jira Issue OCPBUGS-78980, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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
openshift-ci-robot
added
jira/valid-bug
|
@stefanonardo: This pull request references Jira Issue OCPBUGS-78980, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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/knative
|
@stefanonardo: This pull request references Jira Issue OCPBUGS-78980, which is valid. 3 validation(s) were run on this bug
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. |
|
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:
WalkthroughUpdated GraphQL runtime dependencies and GraphQL Code Generator devDependencies in ChangesFrontend GraphQL dependency upgrades
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/packages/knative-plugin/src/components/add/brokers/AddBroker.tsx (1)
45-52:⚠️ Potential issue | 🟠 MajorFix Promise contract violation:
createResourcesreturnsnullon YAML parse failure, buthandleSubmitunconditionally chains.then(), causing a runtime errorWhen invalid YAML is parsed,
createResourcesreturnsnullat line 51, but the function is typed to returnPromise<K8sResourceKind>. This causeshandleSubmitto call.then()onnull, throwing a synchronous TypeError instead of displaying the validation error.Change the return type to
Promise<K8sResourceKind | null>and replace the promise chain with async/await to handle the null case correctly.Proposed fix
- const createResources = ( + const createResources = async ( formValues: AddBrokerFormYamlValues, actions: FormikHelpers<AddBrokerFormYamlValues>, - ): Promise<K8sResourceKind> => { + ): Promise<K8sResourceKind | null> => { @@ - const handleSubmit = ( + const handleSubmit = async ( values: AddBrokerFormYamlValues, actions: FormikHelpers<AddBrokerFormYamlValues>, ) => { - return createResources(values, actions) - .then(() => { - handleRedirect(values.formData.project.name, perspective, perspectiveExtension, navigate); - }) - .catch((err) => { - actions.setStatus({ submitError: err.message }); - }); + try { + const broker = await createResources(values, actions); + if (!broker) { + return; + } + handleRedirect(values.formData.project.name, perspective, perspectiveExtension, navigate); + } catch (err) { + actions.setStatus({ + submitError: err instanceof Error ? err.message : String(err), + }); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/knative-plugin/src/components/add/brokers/AddBroker.tsx` around lines 45 - 52, The createResources function currently returns null on YAML parse failure which violates its Promise<K8sResourceKind> contract; update createResources to return Promise<K8sResourceKind | null> (ensure it always returns a Promise that resolves to null on parse error) and keep the YAML parse/metadata logic that calls safeLoad and actions.setStatus; then update handleSubmit to stop using unconditional .then() chaining on createResources — convert handleSubmit to async/await, await createResources, check for a null result and abort early (displaying the validation error) before proceeding with further logic, so no .then() is invoked on null.frontend/packages/knative-plugin/src/utils/create-channel-utils.ts (1)
169-174:⚠️ Potential issue | 🟠 MajorGuard against malformed
default-ch-configConfigMap during renderLine 26 parses cluster config without error handling. A missing, malformed, or structurally incomplete
default-ch-configwill throw during render and break the Add flow instead of falling back toEVENTING_IMC_KIND. Additionally, lines 29–30 don't fully guard.kindproperty access with optional chaining, so unexpected config structures also cause crashes.The codebase pattern (seen in AddBroker.tsx) shows this error handling is expected. Wrap the parse and property access in try/catch, properly type the config shape, and ensure the fallback is reachable.
Proposed fix
if (configMap && defaultConfiguredChannelLoaded) { - const cfg = safeLoad(configMap.data?.['default-ch-config']) as Record<string, any>; - - defaultConfiguredChannel = _.hasIn(cfg?.namespaceDefaults, namespace) - ? cfg?.namespaceDefaults[namespace].kind - : cfg?.clusterDefault.kind; + try { + const cfg = safeLoad(configMap.data?.['default-ch-config']) as + | { + namespaceDefaults?: Record<string, { kind?: string }>; + clusterDefault?: { kind?: string }; + } + | undefined; + + defaultConfiguredChannel = + (_.hasIn(cfg?.namespaceDefaults, namespace) + ? cfg?.namespaceDefaults?.[namespace]?.kind + : cfg?.clusterDefault?.kind) || EVENTING_IMC_KIND; + } catch { + defaultConfiguredChannel = EVENTING_IMC_KIND; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/knative-plugin/src/utils/create-channel-utils.ts` around lines 169 - 174, The parsing and property access for the ConfigMap `configMap.data['default-ch-config']` (used when `defaultConfiguredChannelLoaded` is true) must be guarded: wrap the `safeLoad` call and subsequent access to `cfg.namespaceDefaults[namespace].kind` / `cfg.clusterDefault.kind` in a try/catch, validate `cfg` against an expected shape (e.g., optional `namespaceDefaults` object and a `clusterDefault.kind` string) before reading properties, use optional chaining for all `.kind` accesses, and on any error or malformed structure set `defaultConfiguredChannel` to the fallback `EVENTING_IMC_KIND` so the Add flow does not crash; update references in this block (variables `cfg`, `defaultConfiguredChannel`, `configMap`, `namespace`, `default-ch-config`) accordingly.
🤖 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/package.json`:
- Line 187: The dependency upgrade to "graphql": "^16.8.0" in package.json is
incompatible with existing peer ranges of apollo-client@2.6.8 and
subscriptions-transport-ws@0.9.16; either downgrade/pin "graphql" to a 14.x
range (e.g., ^14.0.0) to satisfy those peers or upgrade/migrate off
apollo-client (Apollo Client v3+) and subscriptions-transport-ws (or swap to
graphql-ws) to versions that declare compatibility with graphql@16; update
package.json accordingly and run npm/yarn install to verify peer conflicts are
resolved.
In `@frontend/packages/helm-plugin/src/catalog/providers/useHelmCharts.tsx`:
- Around line 54-56: The YAML-to-JSON parsing currently asserts the result has
entries without runtime checks; after calling safeLoad(yaml) (variable json) and
before calling setHelmCharts, validate that json is an object and json.entries
is present and of the expected shape (e.g., non-null/defined), and if not throw
a descriptive Error (e.g., "Helm index missing entries") so the Promise rejects
and your existing .catch() handling runs; update the logic around safeLoad,
json, and setHelmCharts to perform this runtime check rather than relying solely
on the HelmChartEntries type assertion.
---
Outside diff comments:
In `@frontend/packages/knative-plugin/src/components/add/brokers/AddBroker.tsx`:
- Around line 45-52: The createResources function currently returns null on YAML
parse failure which violates its Promise<K8sResourceKind> contract; update
createResources to return Promise<K8sResourceKind | null> (ensure it always
returns a Promise that resolves to null on parse error) and keep the YAML
parse/metadata logic that calls safeLoad and actions.setStatus; then update
handleSubmit to stop using unconditional .then() chaining on createResources —
convert handleSubmit to async/await, await createResources, check for a null
result and abort early (displaying the validation error) before proceeding with
further logic, so no .then() is invoked on null.
In `@frontend/packages/knative-plugin/src/utils/create-channel-utils.ts`:
- Around line 169-174: The parsing and property access for the ConfigMap
`configMap.data['default-ch-config']` (used when
`defaultConfiguredChannelLoaded` is true) must be guarded: wrap the `safeLoad`
call and subsequent access to `cfg.namespaceDefaults[namespace].kind` /
`cfg.clusterDefault.kind` in a try/catch, validate `cfg` against an expected
shape (e.g., optional `namespaceDefaults` object and a `clusterDefault.kind`
string) before reading properties, use optional chaining for all `.kind`
accesses, and on any error or malformed structure set `defaultConfiguredChannel`
to the fallback `EVENTING_IMC_KIND` so the Add flow does not crash; update
references in this block (variables `cfg`, `defaultConfiguredChannel`,
`configMap`, `namespace`, `default-ch-config`) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ab0d5d20-5a5f-4f4e-ad32-1e29595b5635
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
frontend/package.jsonfrontend/packages/console-shared/src/components/editor/yaml-download-utils.tsfrontend/packages/console-shared/src/hooks/useResourceSidebarSamples.tsfrontend/packages/dev-console/src/components/deployments/EditDeployment.tsxfrontend/packages/helm-plugin/src/catalog/providers/useHelmCharts.tsxfrontend/packages/helm-plugin/src/components/forms/HelmChartRepository/CreateHelmChartRepository.tsxfrontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmChartVersionDropdown.tsxfrontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmInstallUpgradePage.tsxfrontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartInstallPage.tsxfrontend/packages/knative-plugin/src/components/add/brokers/AddBroker.tsxfrontend/packages/knative-plugin/src/utils/create-channel-utils.tsfrontend/packages/vsphere-plugin/src/components/persist.tsfrontend/public/components/edit-yaml.tsxfrontend/public/components/monitoring/alertmanager/alertmanager-utils.tsxfrontend/public/components/monitoring/receiver-forms/alert-manager-receiver-forms.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmInstallUpgradePage.tsxfrontend/packages/console-shared/src/hooks/useResourceSidebarSamples.tsfrontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmChartVersionDropdown.tsxfrontend/packages/console-shared/src/components/editor/yaml-download-utils.tsfrontend/packages/helm-plugin/src/catalog/providers/useHelmCharts.tsxfrontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartInstallPage.tsxfrontend/packages/vsphere-plugin/src/components/persist.tsfrontend/packages/helm-plugin/src/components/forms/HelmChartRepository/CreateHelmChartRepository.tsxfrontend/public/components/monitoring/receiver-forms/alert-manager-receiver-forms.tsxfrontend/packages/dev-console/src/components/deployments/EditDeployment.tsxfrontend/public/components/monitoring/alertmanager/alertmanager-utils.tsxfrontend/packages/knative-plugin/src/components/add/brokers/AddBroker.tsxfrontend/package.jsonfrontend/packages/knative-plugin/src/utils/create-channel-utils.tsfrontend/public/components/edit-yaml.tsx
🔇 Additional comments (11)
frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmChartVersionDropdown.tsx (1)
128-128: LGTM — type assertion with runtime safety in place.The cast to
{ entries: HelmChartEntries }is appropriate here. Optional chaining onjson?.entriesdownstream (lines 134, 144, 147) provides adequate runtime protection if the YAML structure deviates unexpectedly.frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/CreateHelmChartRepository.tsx (1)
94-94: LGTM — well-protected type assertion.The cast to
HelmChartRepositoryTypeis safely wrapped in try/catch, and downstream access uses optional chaining onmetadata?.namespace. Error handling properly surfaces YAML parse failures to the user.frontend/packages/console-shared/src/components/editor/yaml-download-utils.ts (1)
5-16: LGTM — defensive coercion with graceful fallback.The
String(data)coercion handles theBlobPartunion type safely. If a non-stringBloborBufferSourcewere passed, thesafeLoadwould fail and the catch block gracefully falls back tok8s-object.yaml. All current call sites pass strings anyway, so this is purely defensive.frontend/public/components/monitoring/alertmanager/alertmanager-utils.tsx (2)
47-47: LGTM — appropriate type assertion.The cast to
AlertmanagerConfigis correctly wrapped in try/catch. Parse errors are surfaced via the error message return path.
58-58: LGTM — explicit type handling for union parameter.Good defensive handling of the
object | stringunion. The_.isObject()check ensures proper YAML serialization for objects whileString(yaml)handles the string case explicitly rather than relying on implicit coercion.frontend/public/components/monitoring/receiver-forms/alert-manager-receiver-forms.tsx (1)
546-546: LGTM — pragmatic type assertion for destructuring.Using
Record<string, any>is appropriate here since we're only extracting theglobalproperty. The try/catch properly handles malformed YAML, and undefinedglobalis acceptable downstream.frontend/public/components/edit-yaml.tsx (4)
77-79: LGTM — template loading with expected type.The cast to
K8sResourceKindaligns with the function's return type and the subsequentmetadataaccess. YAML templates are controlled inputs, so parse failures here indicate a bug in the template itself.
339-339: LGTM — defensive type assertion.Good error handling pattern — returns
'unknown'if parsing fails, preventing cascading errors.
381-381: LGTM — validation function with try/catch.The cast result is unused since this is purely a validity check. Clean pattern.
598-598: LGTM — user input parsing with proper error handling.The cast to
Record<string, any>is appropriate before thevalidate()call, and the try/catch surfaces parse errors to the user viahandleError.frontend/packages/dev-console/src/components/deployments/EditDeployment.tsx (1)
49-49: LGTM — well-guarded type assertion.The cast to
K8sResourceKindis properly wrapped in try/catch, and the subsequentdeploymentRes?.metadata?.namespacecheck uses optional chaining for runtime safety. Error feedback is correctly surfaced to the form status.
| "gherkin-lint": "^4.1.3", | ||
| "git-url-parse": "^11.4.0", | ||
| "graphql": "^14.0.0", | ||
| "graphql": "^16.8.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's verify the package.json file and check line 187 and surrounding context
head -n 200 frontend/package.json | tail -n 30Repository: openshift/console
Length of output: 999
🏁 Script executed:
#!/bin/bash
# Get the full dependency list for apollo and graphql packages
grep -E "(graphql|apollo|subscriptions-transport)" frontend/package.jsonRepository: openshift/console
Length of output: 910
🏁 Script executed:
#!/bin/bash
# Check the client.ts file to confirm Apollo 2 usage
cat -n frontend/public/graphql/client.ts | head -n 50Repository: openshift/console
Length of output: 1847
🏁 Script executed (no clone):
#!/bin/bash
# Check Apollo Client 2.6.8 peer dependencies from npm registry
curl -s https://registry.npmjs.org/apollo-client/2.6.8 | jq '.peerDependencies, .peerDependenciesMeta' 2>/dev/null || echo "Failed to fetch from npm"
Length of output: 95
🏁 Script executed (no clone):
#!/bin/bash
# Also check subscriptions-transport-ws compatibility
curl -s https://registry.npmjs.org/subscriptions-transport-ws/0.9.16 | jq '.peerDependencies, .peerDependenciesMeta' 2>/dev/null || echo "Failed to fetch from npm"
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Check if there are any reported issues or comments about graphql 16 compatibility in the codebase
find . -name "*.md" -o -name "*.txt" | xargs grep -l "graphql.*16\|apollo.*graphql" 2>/dev/null | head -5Repository: openshift/console
Length of output: 43
GraphQL 16 incompatible with Apollo Client 2 peer dependencies
Line 187 upgrades graphql to ^16.8.0, but apollo-client@2.6.8 declares peer dependency graphql@^14.0.0 (max), and subscriptions-transport-ws@0.9.16 declares graphql@^14.0.2 (max). This runtime configuration violates those peer ranges and will cause compatibility issues at runtime.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/package.json` at line 187, The dependency upgrade to "graphql":
"^16.8.0" in package.json is incompatible with existing peer ranges of
apollo-client@2.6.8 and subscriptions-transport-ws@0.9.16; either downgrade/pin
"graphql" to a 14.x range (e.g., ^14.0.0) to satisfy those peers or
upgrade/migrate off apollo-client (Apollo Client v3+) and
subscriptions-transport-ws (or swap to graphql-ws) to versions that declare
compatibility with graphql@16; update package.json accordingly and run npm/yarn
install to verify peer conflicts are resolved.
stefanonardo
force-pushed
the
OCPBUGS-78980
branch
2 times, most recently
from
0702518 to
827b5ae
Compare
|
/assign @logonoff |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
stefanonardo
changed the title
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
@stefanonardo could you please rebase this one 🙏 |
stefanonardo
force-pushed
the
OCPBUGS-78980
branch
from
827b5ae to
af6885a
Compare
stefanonardo
force-pushed
the
OCPBUGS-78980
branch
from
e64290d to
c6633b4
Compare
|
Thanks @stefanonardo 👍 |
|
@jhadvig you're right, I'm updating the description |
|
/test backend |
| "@types/immutable": "^3.8.3", | ||
| "@types/jest": "^30.0.0", | ||
| "@types/js-yaml": "^3.12.7", | ||
| "@types/js-yaml": "3.12.10", |
There was a problem hiding this comment.
Why are we pinning us to a specific version ?
There was a problem hiding this comment.
actualy there is no reason to update this. When I wrote this PR we didn't have this entry in package.json
| import { useUserPreference } from './useUserPreference'; | ||
|
|
||
| export interface ClusterProperties { | ||
| interface ClusterProperties { |
There was a problem hiding this comment.
We are unexporting this interface? No objection just seems irrelevant to the overall change.
There was a problem hiding this comment.
frontend job was failing because this export was unused
stefanonardo
force-pushed
the
OCPBUGS-78980
branch
2 times, most recently
from
7e638a7 to
839ea4f
Compare
stefanonardo
force-pushed
the
OCPBUGS-78980
branch
from
839ea4f to
71c9b0d
Compare
stefanonardo
force-pushed
the
OCPBUGS-78980
branch
2 times, most recently
from
770f63a to
bdf978a
Compare
|
/test backend |
1 similar comment
|
/test backend |
stefanonardo
force-pushed
the
OCPBUGS-78980
branch
3 times, most recently
from
5ec1b91 to
057bc78
Compare
|
/retest |
|
/test backend |
…ibility The console build fails on Node.js 25 because @graphql-codegen/cli@1.x transitively depends on buffer-equal-constant-time, which uses the removed SlowBuffer API. Upgrading to codegen v5 eliminates this dependency chain.
stefanonardo
force-pushed
the
OCPBUGS-78980
branch
from
057bc78 to
2032958
Compare
|
@stefanonardo: 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. |
jhadvig
added
the
plugin-api-approved
jhadvig
added
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, stefanonardo 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 |
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@jhadvig: 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. |
6 participants
Summary
@graphql-codegen/clifrom v1 to v5 (and related codegen packages) to eliminate the transitive dependency onbuffer-equal-constant-time, which uses theSlowBufferAPI removed in Node.js 25graphqlfrom^14.0.0to^15.0.0andgraphql-tagto^2.12.6(required peer deps for codegen v5, while staying compatible withapollo-client@2.xpeer range)Test plan
yarn buildcompiles with 0 errorsbuffer-equal-constant-timeno longer present inyarn.lockyarn generate-graphqlproduces equivalent generated types (exported type names unchanged)🤖 Generated with Claude Code