Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 9 commits
4862fa0
ecc7e50
d1f3aa2
4316970
519adfd
618f538
f8fa8fe
a976eb6
d853004
2dd4e97
018789a
3d9b41c
7305016
6491cbb
5f051c9
e1b938b
ae52b83
46b9cda
4f12b2b
2ab2ea9
c34df9d
2f669da
a7ac1fd
f650e1f
f8c0aac
598ca1c
848b20c
48dcff8
b5420ad
bcf46b6
a4d5a21
64c3faa
7e51da5
88ea796
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.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.
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 onordinal
. How do we want to address this? I can see a few approaches: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.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.
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 oneDisplayUrlProvider
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.
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
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.