Bug 1775741: Re-start cola layout when nodes are added #3537
Conversation
openshift-ci-robot
added
component/dev-console
jeff-phillips-18
changed the title
|
@jeff-phillips-18: This pull request references Bugzilla bug 1775741, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. In 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 kubernetes/test-infra repository. |
openshift-ci-robot
added
the
bugzilla/valid-bug
|
/bugzilla refresh |
|
@jeff-phillips-18: This pull request references Bugzilla bug 1775741, which is valid. In 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 kubernetes/test-infra repository. |
|
/cherrypick release-4.3 |
|
@jeff-phillips-18: once the present PR merges, I will cherry-pick it on top of release-4.3 in a new PR and assign it to you. In 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 kubernetes/test-infra repository. |
| @@ -36,6 +36,8 @@ class ColaNode implements webcola.Node { | |||
|
|
|||
| public parent: ColaGroup; | |||
|
|
|||
| public fixed: number = 0; | |||
There was a problem hiding this comment.
Curious, why fixed next to a property called isFixed. Based on what I see, fixed is only ever set to 0 or 1... a boolean?
There was a problem hiding this comment.
isFixed we use locally for the force simulation.
fixed is used by Cola and is a number (they use different bits but consumers use only the first bit).
There was a problem hiding this comment.
Fair - would you be able to put a comment to that extent?
The code looks a little odd with isFixed and fixed both as properties on the same class.
There was a problem hiding this comment.
will do (nothing I like more than typing /retest a hundred times)
There was a problem hiding this comment.
I'm sorry. Wasn't my intent to cause trouble. The variables are confusing without context.
There was a problem hiding this comment.
heh, I was just kidding 😉
jeff-phillips-18
force-pushed
the
topology-cola
branch
from
c7c27da
to
86b3d85
Compare
openshift-ci-robot
added
lgtm
|
@jeff-phillips-18: This pull request references Bugzilla bug 1775741, which is valid. In 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 kubernetes/test-infra repository. |
There was a problem hiding this comment.
Not sure if it's directly related to your latest work (don't spend a lot of time in Topology) but I ran into an odd layout when nodes are added to a group that possess a connection.
EDITs:
- Only occurs on "layout 1"
- Seems to only occur when it's vertical like the above screenshots... horizontal doesn't appear to have this issue.
| const prevGroup = prevGroups.find((g) => g.element.getId() === group.element.getId()); | ||
| if (!prevGroup) { | ||
| addingNodes = true; | ||
| newNodes.push(...group.leaves); |
There was a problem hiding this comment.
This will mutate this.nodes when you're on your initialRun. Not sure if it matters, just figured I'd mention it.
There was a problem hiding this comment.
it's not obvious but initialRun and restart are never both true. I can update with a comment or a check. Thanks.
|
/retest |
jeff-phillips-18
force-pushed
the
topology-cola
branch
from
6efaa40
to
0eac3da
Compare
|
/lgtm |
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
4 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
9 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
7 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@jeff-phillips-18: All pull requests linked via external trackers have merged. Bugzilla bug 1775741 has been moved to the MODIFIED state. In 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 kubernetes/test-infra repository. |
|
@jeff-phillips-18: new pull request created: #3603 In 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 kubernetes/test-infra repository. |
Resolves https://jira.coreos.com/browse/ODC-1884
Resolves https://jira.coreos.com/browse/ODC-2417
This uses the Cola layout on newly added nodes and keeps existing items in their current location (with the exception of constrained nodes like knative revisions and event sources).