-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix YAML Config Editing Console Runtime Errors by Switching to Yaml Library #289
Changes from 3 commits
a68d802
b0745e6
8b2d4d4
e7d25c4
5981148
f0e54e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,7 @@ | ||
// SPDX-FileCopyrightText: 2023 Dash0 Inc. | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
import { Parser } from "yaml"; | ||
import JsYaml, { FAILSAFE_SCHEMA } from "js-yaml"; | ||
import YAML, { Parser } from "yaml"; | ||
export interface SourceToken { | ||
type: | ||
| "byte-order-mark" | ||
|
@@ -240,8 +239,10 @@ export function isOtelColCRD(jsonData: IOtelColCRD) { | |
} | ||
|
||
export function selectConfigType(config: string) { | ||
const jsonData = JsYaml.load(config, { schema: FAILSAFE_SCHEMA }) as any; | ||
|
||
let jsonData: any; | ||
try { | ||
jsonData = YAML.parse(config, { logLevel: "error", schema: "failsafe" }) as any; | ||
} catch (e) {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: This silently catches and fully ignores the error. That seems dangerous to me. Why do we need to do this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought we were already displaying errors in the validation console; when editing the config, the console is overflowing with errors. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please document this in the code in both locations? Because this empty catch block very much looks like a bug otherwise :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The related comments added to the code |
||
if (isK8sConfigMap(jsonData)) { | ||
return jsonData.data.relay; | ||
} else if (isOtelColCRD(jsonData)) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ import React, { type RefObject, useEffect, useMemo } from "react"; | |
import ReactFlow, { Background, Panel, useReactFlow, useNodesState, useEdgesState, useStore } from "reactflow"; | ||
import "reactflow/dist/style.css"; | ||
import type { IConfig } from "./dataType"; | ||
import { Parser } from "yaml"; | ||
import YAML, { Parser } from "yaml"; | ||
import useEdgeCreator from "./useEdgeCreator"; | ||
import { useFocus } from "~/contexts/EditorContext"; | ||
import { Minus, Plus, HelpCircle, Lock, Minimize2 } from "lucide-react"; | ||
|
@@ -22,7 +22,6 @@ import ReceiversNode from "./node-types/ReceiversNode"; | |
import ProcessorsNode from "./node-types/ProcessorsNode"; | ||
import { useLayout } from "./layout/useLayout"; | ||
import CyclicErrorEdge from "./CyclicErrorEdge"; | ||
import JsYaml, { FAILSAFE_SCHEMA } from "js-yaml"; | ||
|
||
type EditorRefType = RefObject<editor.IStandaloneCodeEditor | null>; | ||
|
||
|
@@ -40,7 +39,13 @@ export default function Flow({ | |
editorRef: EditorRefType | null; | ||
}) { | ||
const reactFlowInstance = useReactFlow(); | ||
const jsonData = useMemo(() => JsYaml.load(value, { schema: FAILSAFE_SCHEMA }) as IConfig, [value]); | ||
|
||
const jsonData = useMemo(() => { | ||
try { | ||
return YAML.parse(value, { logLevel: "error", schema: "failsafe" }) as IConfig; | ||
} catch (error: unknown) {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question as above. |
||
}, [value]) as IConfig; | ||
|
||
const pipelines = useMemo(() => { | ||
const parsedYaml = Array.from(new Parser().parse(value)); | ||
const doc = parsedYaml.find((token) => token.type === "document") as Document; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Why are we using the short version here? IMHO that is hard to understand, and we seem have enough room in the console for the long version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, it will be change to the long version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The long version message is pushed and committed.