-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
[JENKINS-71715] Allow plugins with alternate build log visualizations to handle console links from core and other plugins #8321
[JENKINS-71715] Allow plugins with alternate build log visualizations to handle console links from core and other plugins #8321
Conversation
… to handle console links from core and other plugins
Yay, your first pull request towards Jenkins core was created successfully! Thank you so much! |
public String getRoot() { | ||
String root = Jenkins.get().getRootUrl(); | ||
if (root == null) { | ||
root = "http://unconfigured-jenkins-location/"; |
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.
Is there a legitimate use case for when this code would be reached?
Display URL API has this because the URLs are used outside Jenkins and computed in a non-request context. It seems these URLs are always used in a request context, as they're used to render parts of the UI?
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.
Good point, it would probably be better to design the new API to work with URLs relative to the context root, and then use something like '/' + Stapler.getCurrentRequest().getContextPath() + getConsoleURL(run)
to build the console URLs.
The implementation of the extension that should go into jenkinsci/display-url-api-plugin#194 would need to convert the absolute URLs into context path-relative URLs though, and I guess what I was thinking here would no longer apply, since we would want to use ConsoleURLProvider
for all links in the Jenkins UI and DisplayUrlProvider.getConsoleUrl
only when producing external links.
EDIT: Although now I wonder if there is a use case where people want to redirect to a completely separate external logging system, e.g. maybe you use opentelemetry
as described here and want to direct users to some Elasticsearch frontend instead.
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.
EDIT: Although now I wonder if there is a use case where people want to redirect to a completely separate external logging system
That seems like something that could be interesting. Summoning @cyrille-leclerc WDYT?
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.
Thinking about it some more -- just after the ping comment, sorry Cyrille -- this could also easily be implemented by having a Jenkins-internal URL that redirects to the outside system, instead of linking there directly.
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.
Thanks @daniel-beck. Regarding the OpenTelemetry integrations where Jenkins pipeline logs are persisted in an external logs management system rather than in the Jenkins home, all integrations I am aware of want to
- Use Jenkins's built-in pipeline logs visualization UX to render the logs stored externally
- Offer a visualization in their own logs management GUI, often workflows combining metrics, traces, and logs of the pipeline executions.
I don't see any OTel integration scenario wanting to create an alternative pipeline logs visualization GUI in Jenkins.
Does it make sense?
@@ -0,0 +1,59 @@ | |||
package hudson.link; |
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.
Please use jenkins.…
packages for new types.
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.
Is it better to create a jenkins.console
package and put it there or put it in the already-existing hudson.console
package?
we usually don't put versions but put todo instead and these versions get resolved _after_ merges. Co-authored-by: Daniel Beck <1831569+daniel-beck@users.noreply.github.com>
Note to self that if merged, #7633 could take advantage of this API. Currently it just redirects to the index page of the new build, but it would be better to go straight to the log. |
…ons.getContextRelativeUrl
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.
How would this be used by an implementation? Given this looks at the highest priority implementation in the static extension list, do admins need https://plugins.jenkins.io/extension-filter/ to configure that? Or is the expectation really that nothing but display-url-api
implements this? The answer should be documented in Javadoc.
I am going to change things. I think I'll swap to looping over all extensions, and then allowing implementations to return |
…ConsoleUrl method for use in Jelly files
* @param run the build | ||
* @return the URL for the console for the specified build, relative to the web server root | ||
*/ | ||
static @NonNull String getRedirectUrl(Run<?, ?> run) { |
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 method is intended to be used directly in cases where Java code wants to redirect a request to the console as in jenkinsci/pipeline-input-step-plugin#145.
<l:task href="${h.getConsoleUrl(it)}" icon="icon-terminal icon-md" title="${%Console Output}"/> | ||
<l:task href="${buildUrl.baseUrl}/consoleText" icon="icon-document icon-md" title="${%View as plain text}"/> |
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.
I am not entirely sure what we want to do about this case. I think it makes sense to redirect the main "console output" link, but IDK if we should force-show the "View as plain text" link when the console URL is non-default or something like that.
* @return the absolute URL for accessing the specified build's console | ||
* @since TODO | ||
*/ | ||
public static @CheckForNull String getConsoleUrl(Run<?, ?> run) { |
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 method is intended for use in Jelly files (both in core and in plugins). I think it is a little cleaner to put it here rather than directly on Run
, but that is just my opinion.
*/ | ||
static @NonNull String getRedirectUrl(Run<?, ?> run) { | ||
String url = null; | ||
for (ConsoleUrlProvider provider : ExtensionList.lookup(ConsoleUrlProvider.class)) { |
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.
@daniel-beck raised a few important points in #8321 (review) and I am not entirely sure how we should address them.
With my latest changes to the API, we can create implementations that only care about Pipelines, or builds with odd numbers, etc. However, if (for example) both Blue Ocean and Pipeline Graph View wanted to implement ConsoleUrlProvider
for all Pipeline builds, and you have both plugins installed on a single controller, they would compete for precedence based on ordinal
. How do we want to address this? I can see a few approaches:
- Do nothing, let admins use https://plugins.jenkins.io/extension-filter/ as needed. Maybe this is good enough if we only expect a few overlapping implementations of this extension point, especially if we think the implementations are likely to come from competing plugins where admins are going to choose one instead of installing them all at the same time.
- Provide a
GlobalConfiguration
UI in core to allow admins to select whichConsoleUrlProvider
extensions should be enabled on a given controller, or allow them to be sorted based on the admin's preference. Same end result as above, though arguably more user-friendly, but it adds additional implementation complexity. - Provide a user-based configuration in core to let users select which
ConsoleUrlProvider
extensions they want to allow, or allow them to be sorted based on the user's preference. Adds even more implementation complexity than the above options, but now if there are two plugins installed with competingConsoleUrlProvider
implementations, individual users get to pick which one they want to use. (I think all of the links in question are generated dynamically during page rendering/request handling and so would be able to use the current user's preferences without significant technical issues, but perhaps it would be more complex than that.)
For now at least I need to update jenkinsci/pipeline-graph-view-plugin#288 and jenkinsci/pipeline-input-step-plugin#145 based on the latest changes.
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.
I suspect you will want 2 and 3, admins to set a default and allow users to override it
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.
Missed this thread and posted #8321 (comment). For now IMO option 2 should be enough (option 3 only if sufficient user demand exists).
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.
I feel you'll get users that are used to it but some users who would want the other implementation, also with the display url only having a per user setting it seems weird for this one not to?
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.
with the display url only having a per user setting
For what it's worth, related to this work we will add a global configuration to display-url-api
to allow admins to pick the default setting for users who have not yet configured their own preference. (Right now I think the default behavior is effectively random if you have more than one DisplayUrlProvider
extension on a controller.)
As far as whether we want to allow per-user settings here, my main hesitation is just that it adds complexity to core and I am not sure how useful it will be in practice. For the record, I don't know if it makes sense to actually implement the new API in Blue Ocean because I don't think that it is a sufficient replacement for the main console view in general.
That leaves pipeline-graph-view
as the only OSS visualization plugin that I was planning on filing a PR against for now (let me know if I am missing something obvious). Beyond that, CloudBees has a proprietary visualization plugin that we would like to have implement the new API for most Pipelines. All that to say, I am not sure how often conflicts would occur in practice, at least for now.
If you see this API as one intended for admins to control the UX of their instance and guide users to a preferred console visualization, then having a global configuration alone seems fine. If you think it is just a matter of personal preference then a per-user configuration makes sense.
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.
For a cloudbees plugin it may make more sense as a control of the whole instance.
In the case of pipeline graph view plugin someone may want to fallback to the default console view if it doesn't handle something for them or they have used it for 10+ years and do not want to change.
also PGV doesn't support freestyle builds at all and I haven't seen any handling for that in the downstream PR.
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.
also PGV doesn't support freestyle builds at all and I haven't seen any handling for that in the downstream PR.
Yes sorry, all of the downstream PRs need to be updated. I will get to that later today.
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.
See 018789a and let me know what you think. We can dial it back as desired.
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 to me
I'm wondering what the configuration would look like then if you end up with two implementations. Find out whichever is the higher precedence (how?) and disable that in a plugin-defined manner, so it falls through to the other, seems like an awkward admin experience. A global configuration that only shows up when you have a non-default implementation (similar to Artifact Management for Builds) might be a reasonable UI for this: Admins add/remove/sort descriptors of what's not the default. Could even gracefully reduce to an enable/disable checkbox if there's just one implementation. To clarify, the above suggestion is not a requirement, but I think it's important for this PR to provide an answer to how two implementations interact / how to configure them. I don't think you want to go the way of |
Noting #8403 which is probably relevant for this PR. |
core/src/main/java/jenkins/console/ConsoleUrlProviderGlobalConfiguration.java
Show resolved
Hide resolved
…ser providers fail to provide a valid URL
Ok, should I go ahead and move the new global configuration from this PR into that category? I think it is conceptually very similar to
|
Yes please |
It's not that easy though, otherwise we get Since both category and this extension point are a core feature, this PR could add special behavior for this case in jenkins/core/src/main/java/jenkins/appearance/AppearanceGlobalConfiguration.java Lines 56 to 57 in 9dabd87
|
…derGlobalConfiguration is disabled and would be the only option on the page
I just noticed that "Appearance" page is still reachable through direct URL |
I'm not 100% sure, but I don't think there is a simple way to do it. That behavior should be the same though with or without this PR: if you install the latest Jenkins weekly, do not install any plugins, and go to |
👍 I wrongly thought the "Appearance" page was introduced by this PR. Then everything looks good to me. |
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.
Nice feature
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form"> | ||
<j:if test="${descriptor.enabled}"> | ||
<f:invisibleEntry> | ||
<!-- Dummy field to always have an entry in form submissions so users can delete all providers. --> |
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.
how I hate that ...
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.
Do not do it this way. Rather start doConfigSubmit
or whatever by nulling out the field before binding properties.
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.
IIRC, both were needed, otherwise configure
didn't even get called. I will check again though, perhaps this can be deleted.
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.
Ah yeah I think this behavior was specific to user properties actually. I'll still check it again though.
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.
Probably what you ran into is that form binding only provides a JSON element for a list if it has at least one entry. So for a top-level (reconfigurable) object, if you just use the default bindJSON
, after deleting all elements of a list and saving, no change is applied to that property. The workaround is to simply null out any list-valued fields before calling bindJSON
.
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.
For ConsoleUrlProviderGlobalConfiguration
, yes, that was a problem and that is how I fixed it.
ConsoleUrlProviderUserProperty
is different though. User.doConfigSubmit
is unusual and has complex logic regarding creating new properties vs reconfiguring existing ones. IIRC, in the problematic case, since ConsoleUrlProviderUserProperty
only has a single List
field, this condition is (EDIT) false
because the property itself is not present in the submitted JSON at all, and so the logic there chooses to reuse the existing property if one exists.
Whether that logic was intentional was not clear to me. Perhaps these lines should be moved inside of this block, so that any properties not present in the submission are deleted, but I have no idea if anything relies on the current behavior (perhaps this case is intended to preserve user properties that do not offer any GUI configuration?) and it was not something I wanted to look into here.
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.
perhaps this case is intended to preserve user properties that do not offer any GUI configuration?
Perhaps. (Or those whose GUI configuration is on a separate page.)
is
if (o != null) { true
because the property itself is not present in the submitted JSON at all
false
you mean? Smells like a bug in the form submission JSON generator; it should at least create {}
maybe with _class
.
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.
Yes sorry I meant false
.
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.
Works fine in testing with the downstream PRs. Code and tests look reasonable as well.
Some (optional) thoughts:
- It'd be useful to have developer guidelines for implementations. For example, implementing plugins no longer need a separate link to their own console (as the current PR of
pipeline-graph-view
still does), but OTOH would be good for them to a link to the legacy build log in their custom UI. - The current wording focuses on the URLs/links because that's what it does from a technical POV. From a user POV, it seems to me it's more about the chosen build log visualization. Perhaps someone more in the PM/PO/UX space can provide feedback on the name of this?
core/src/main/resources/jenkins/console/ConsoleUrlProviderUserProperty/config.properties
Outdated
Show resolved
Hide resolved
core/src/main/resources/jenkins/console/ConsoleUrlProviderGlobalConfiguration/config.properties
Outdated
Show resolved
Hide resolved
core/src/main/resources/jenkins/console/ConsoleUrlProviderGlobalConfiguration/config.jelly
Show resolved
Hide resolved
Sure, we could change "Console URL Provider" to "Console Visualization Provider" at least in the user-facing text, and also the class names if desired. |
I will also add a note for implementors to the
I don't think this is so clear-cut. The console visualization plugins cannot trivially tell whether the current user has their visualization selected to know if the link is redundant (they could consult |
…, and filter out default provider in global config Co-authored-by: Daniel Beck <daniel-beck@users.noreply.github.com>
core/src/main/java/jenkins/console/ConsoleUrlProviderGlobalConfiguration.java
Outdated
Show resolved
Hide resolved
This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. /label ready-for-merge |
@daniel-beck do you think I should make this change, or do you have any other suggestions? |
Depends on whether you think this is easy followup, I guess (assuming you agree it would be a worthwhile change)? As I wrote, it's optional. |
Yes it is easy to change later (assuming we do not care about the class names). If you think "Console visualization provider" is better than "Console URL provider" though then I figure we might as well go ahead and change the text on the configuration pages here. |
See JENKINS-71715.
This PR introduces a new extension point
ConsoleUrlProvider
, which is intended to be used by Jelly files that link to the build console and plugins that redirect to the build console, in order for plugins which provide special build console visualizations (such as Pipeline: Graph View and Blue Ocean) to be able to serve their own visualizations in those contexts.See downstream PRs here:
ConsoleUrlProvider
: [JENKINS-71715] Update minimum supported Jenkins version to 2.440 to be able to implement newConsoleUrlProvider
API from Jenkins core pipeline-graph-view-plugin#289Functions.getConsoleUrl
from a plugin: [JENKINS-71715] Update minimum supported Jenkins version to 2.440 to be able to use newConsoleUrlProvider
API to link to Pipeline build console page workflow-job-plugin#373ConsoleUrlProvder.getRedirectUrl
: [JENKINS-71715] Update minimum supported Jenkins version to 2.440 to be able to use newConsoleUrlProvider
extension point to redirect to the console pipeline-input-step-plugin#145I reviewed plugins in https://github.com/jenkinsci/bom for console links to see how they are affected by these changes, see #8321 (comment).
Testing done
New automated test added.
You can manually test the PR by running jenkinsci/pipeline-graph-view-plugin#289 by itself. Note that you will have to configure "Console URL Provider" in either the global configuration or your user configuration for anything to happen.
Here is a video of me testing the downstream changes with a Pipeline build:
Screen.Recording.2023-08-09.at.1.15.52.PM.mov
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
@Restricted
or have@since TODO
Javadocs, as appropriate.@Deprecated(since = "TODO")
or@Deprecated(forRemoval = true, since = "TODO")
, if applicable.eval
to ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@dwnusbaum
Before the changes are marked as
ready-for-merge
:Maintainer checklist
upgrade-guide-needed
label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidate
to be considered (see query).