-
Notifications
You must be signed in to change notification settings - Fork 110
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
Inherit tags from nodes for modular pipelines #1542
Conversation
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.
This looks good. I do not see any lag in fetching tags but if we decide to update tags on FE, this should be good. I left a [Nit] comment for a method name but its non blocking.
src/selectors/nodes.js
Outdated
/** | ||
* Retrieves tags associated with both nodes and their corresponding modular pipelines. | ||
*/ | ||
export const getTaggedNodesAndModularPipelines = createSelector( |
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.
can we rename this to getTagsForNodesAndModularPipelines ?
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.
This makes more sense in the FE than in BE I am happy to go with this solution. 👍
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.
Looks good and works well!
Two small nit comments from me. Also, make sure to sign your commits and make sure the Cypress tests are passing. For some reason, they're failing on your branch.
/** | ||
* Retrieves tags associated with both nodes and their corresponding modular pipelines. | ||
*/ | ||
export const getTagsforNodesAndModularPipelines = createSelector( |
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.
export const getTagsforNodesAndModularPipelines = createSelector( | |
export const getTagsForNodesAndModularPipelines = createSelector( |
const modularPipelineIDs = nodeModularPipelines[nodeID] || []; | ||
modularPipelineIDs.forEach((modularPipelineID) => { |
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.
const modularPipelineIDs = nodeModularPipelines[nodeID] || []; | |
modularPipelineIDs.forEach((modularPipelineID) => { | |
const modularPipelineIDs = nodeModularPipelines[nodeID] || []; | |
modularPipelineIDs.forEach((modularPipelineID) => { |
Signed-off-by: ravi-kumar-pilla <ravi_kumar_pilla@mckinsey.com>
Description
Resolves #795
Development notes
We tried solving this issue in the backend but the way modular pipelines is computed in the backend is very complex. I thought maybe we can try solving it in the FE using selectors.
In this PR, we have a new selector called getTagsforNodesAndModularPipelines.This selector identifies nodes associated with specific tags and then propagates these tags to the corresponding modular pipeline(s) (nested as well) for each node.
QA notes
I have also added some tags to the demo project so we can test this.
Checklist
RELEASE.md
file