Skip to content

Stop adding TimingAction in WorkflowRun$GraphL and let workflow-cps handle it instead#548

Draft
dwnusbaum wants to merge 2 commits into
jenkinsci:masterfrom
dwnusbaum:check-timingaction
Draft

Stop adding TimingAction in WorkflowRun$GraphL and let workflow-cps handle it instead#548
dwnusbaum wants to merge 2 commits into
jenkinsci:masterfrom
dwnusbaum:check-timingaction

Conversation

@dwnusbaum

@dwnusbaum dwnusbaum commented Jul 1, 2025

Copy link
Copy Markdown
Member

Requires jenkinsci/workflow-cps-plugin#1052 to be release first, and then when this is released, users need to make sure that workflow-cps is updated either before or at the same time that they update to this PR, otherwise FlowStartNode will end up without a TimingAction.

While trying to write tests for jenkinsci/workflow-cps-plugin#1052 and #547, I realized that the way that WorkflowRun$GraphL adds TimingAction is almost totally redundant due to FlowHead.setNewHead already adding TimingAction. The TimingAction addition logic was added to FlowHead.setNewHead in jenkinsci/workflow-cps-plugin#188, and then the if was added to the logic here in #75 when picking up the workflow-cps PR in tests to avoid adding a second TimingAction.

The logic in workflow-job is almost totally unused now, since setNewHead is responsible for calling notifyNewHead, which is what notifies GraphListeners, so it already handles almost all nodes. The only case that still matters is FlowStartNode, because newStartNode is separate from setNewHead, and newStartNode doesn't add TimingAction. jenkinsci/workflow-cps-plugin#1052 now handles FlowStartNode directly as of jenkinsci/workflow-cps-plugin@4241907.

Testing done

I tested this PR against the new automated tests in jenkinsci/workflow-cps-plugin#1052, to ensure that with this PR and without the changes to FlowHead.newStartNode in that PR, the new test CpsFlowExecutionTest.timingActionAlwaysAdded fails. (I could duplicate the same test here, but it seems clearest to test over in workflow-cps since that plugin is the one adding TimingAction.)

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests that demonstrate the feature works or the issue is fixed

@dwnusbaum

This comment was marked as outdated.

@dwnusbaum dwnusbaum changed the title Check if WorkflowRun$GraphL still needs to add TimingAction Stop adding TimingAction in WorkflowRun$GraphL and let workflow-cps handle the action for all FlowNodes Jul 2, 2025
@dwnusbaum dwnusbaum changed the title Stop adding TimingAction in WorkflowRun$GraphL and let workflow-cps handle the action for all FlowNodes Stop adding TimingAction in WorkflowRun$GraphL and let workflow-cps handle it instead Jul 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants