Skip to content
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

Vindigo cumulus 3184 #1124

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open

Vindigo cumulus 3184 #1124

wants to merge 12 commits into from

Conversation

Jkovarik
Copy link
Member

@Jkovarik Jkovarik commented Feb 15, 2024

Pulling in #1123

Summary: Summary of changes

Addresses CUMULUS-3184: Dependency d3-dag code integration to d3 graph

Changes

  • Replaced dagre-d3 with dagre-d3-es to address the d3-color vulnerability
  • Upgraded d3 to v7.2.0 to be compatible with dagre-d3-es v7.0.10

PR Checklist

  • Update CHANGELOG
  • Unit tests
  • Adhoc testing
  • Integration tests

@@ -25,13 +25,14 @@ const ExecutionStatusGraph = ({ executionStatus }) => {
const workflow = JSON.parse(stateMachine.definition);
const events = getExecutionEvents(executionHistory);
const graph = workflowToGraph(workflow);
console.log('graph', graph);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it should be removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line removed.

@jjmccoy jjmccoy added PR Feedback Needs Review Looking for a reviewer and removed PR Feedback labels Feb 16, 2024
@npauzenga npauzenga added In Review and removed Needs Review Looking for a reviewer labels Feb 21, 2024
Copy link
Contributor

@npauzenga npauzenga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just a couple clarifying questions but I don't see an issue as long as tests pass.

.nvmrc Show resolved Hide resolved
package.json Outdated
@@ -149,11 +149,10 @@
"compare-versions": "^4.1.2",
"connected-react-router": "^6.9.2",
"console-browserify": "^1.2.0",
"core-js": "^3.20.1",
"core-js": "^3.35.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this also due to the dagre update or a separate concern? Fine either way, just clarifying.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change was to avoid conflict with develop.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like a merge conflict? It's probably fine to update a package like this but what was the conflict?

package.json Outdated
@@ -171,7 +170,7 @@
"path-browserify": "^1.0.1",
"postcss": "^8.4.5",
"process": "^0.11.10",
"prop-types": "^15.8.0",
"prop-types": "^15.8.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question. Was this a vulnerability?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change was to avoid conflict with develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants