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

Delete DownstreamJobListener and instead create NodeDownstreamBuildAction dynamically during graph processing based on DownstreamBuildAction #2540

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

dwnusbaum
Copy link
Member

@dwnusbaum dwnusbaum commented Feb 1, 2024

DownstreamJobListener here has the same bug as jenkinsci/pipeline-build-step-plugin#127 for completed builds. Prior to jenkinsci/workflow-cps-plugin#807, this only worked as long as you were using the MAX_SURVIVABILITY durability level, otherwise it did nothing when the build was already complete. jenkinsci/workflow-cps-plugin#826 then made it so a warning was logged instead of the save being silently ignored in the problematic case, which can lead to log spam.

Untested, because right now Blue Ocean doesn't build for me (I think the main problem is Cannot download "https://github.com/sass/node-sass/releases/download/v7.0.0/darwin-x64-64_binding.node": when trying to build the frontend code, and IDK if there are any releases that support ARM-based Macs: sass/node-sass#3033).

Submitter checklist

  • Link to JIRA ticket in description, if appropriate.
  • Change is code complete and matches issue description
  • Appropriate unit or acceptance tests or explanation to why this change has no tests
  • Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.

Reviewer checklist

  • Run the changes and verified the change matches the issue description
  • Reviewed the code
  • Verified that the appropriate tests have been written or valid explanation given

@dwnusbaum dwnusbaum requested a review from jglick February 1, 2024 18:07
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Can we not just delete (or deprecate) NodeDownstreamBuildAction and change whatever code is calling is to look up org.jenkinsci.plugins.workflow.support.steps.build.DownstreamBuildAction on demand?

@jglick
Copy link
Member

jglick commented Feb 1, 2024

// Actions from any child nodes
actions.addAll(node.getPipelineActions(NodeDownstreamBuildAction.class));
+
downstreamRuns = this.pager.currentNode.actions
.filter(action => action._class === 'io.jenkins.blueocean.listeners.NodeDownstreamBuildAction')
.map(action => ({ runDescription: action.description, runLink: action.link.href }));
IIUC

@dwnusbaum
Copy link
Member Author

Can we not just delete (or deprecate) NodeDownstreamBuildAction and change whatever code is calling is to look up org.jenkinsci.plugins.workflow.support.steps.build.DownstreamBuildAction on demand?

Yes I considered this kind of approach, but I think it would only move the performance issue to graph loading time (a bit better at least since it wouldn't affect non-Blue Ocean use cases). I can switch to it though.

@dwnusbaum
Copy link
Member Author

Oh, and I'm not sure if you can just change the code that looks for the action in the special Blue Ocean FlowNodeWrapper class in PipelineNodeImpl. I think you need to change the code that sets the actions when the graph is being processed so that the action propagates up to the node that is visible in Blue Ocean correctly.

@dwnusbaum dwnusbaum changed the title Replace DownstreamJobListener with a TransientActionFactory based on DownstreamBuildAction Delete DownstreamJobListener and instead create NodeDownstreamBuildAction dynamically during graph processing based on DownstreamBuildAction Feb 1, 2024
@dwnusbaum
Copy link
Member Author

See b7ef607 for an approach that moves the logic to graph processing time.

Delaying build loading until a user actually clicks on one of the relevant links does not seem possible without UX changes, since we expect the description of the run to be available in the UI.

@jglick
Copy link
Member

jglick commented Feb 1, 2024

it wouldn't affect non-Blue Ocean use cases

Which is pretty important I think, since B.O. might be installed yet rarely used.

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

OK AFAICT

Comment on lines +40 to +47
<version>6.8.0.202311291450-r</version>
<exclusions>
<exclusion>
<!-- Provided by Jenkins core -->
<groupId>commons-codec</groupId>
<artifactId>commons-codec</artifactId>
</exclusion>
</exclusions>
Copy link
Member Author

@dwnusbaum dwnusbaum Feb 7, 2024

Choose a reason for hiding this comment

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

FWIW, it is awkward for me to check how much work this would be because I cannot build Blue Ocean, but I think the better fix would be to depend on git-client instead and adjust the code as needed to use Apache Mina to match jenkinsci/git-client-plugin#956.

Copy link
Member Author

@dwnusbaum dwnusbaum Feb 7, 2024

Choose a reason for hiding this comment

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

The code in GitUtils uses JGit's Jsch library directly, and it is not immediately obvious to me how to adapt the code to use higher level APIs or to switch to JGit's Apache Mina APIs, so I will leave this as-is.

@jglick
Copy link
Member

jglick commented Feb 7, 2024

Blue Ocean doesn't build for me

If you do not need to test the GUI interactively, you can just use https://github.com/eirslett/frontend-maven-plugin?tab=readme-ov-file#skipping-execution (which will be a lot faster too).

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