-
Bug
-
Resolution: Fixed
-
Minor
-
-
display-url-api 2.3.9
Context
The GitHub Checks plugin calls DisplayURLProvider.get() and then uses DisplayURLProvider.getRunURL and DisplayURLProvider.getJobURL to get URLs that it publishes to GitHub. These URLs normally end with "display/redirect". When a user clicks such a link on GitHub, the Display URL API plugin redirects to Blue Ocean UI or to the classic UI. If Blue Ocean has been installed, then that is the default, but logged-in users can configure their user account and choose a different option under "Notification URL".
In https://github.com/jenkins-infra/helpdesk/issues/2833, they wanted to change the default so that users would be redirected to the classic UI by default, but logged-in users would still be able to choose Blue Ocean. To implement this, https://github.com/jenkins-infra/jenkins-infra/pull/2143 added -Djenkins.displayurl.provider=org.jenkinsci.plugins.displayurlapi.ClassicDisplayURLProvider to the Java startup options.
Problem
After the jenkins.displayurl.provider property had been set, the URLs that were published to GitHub no longer included the "display/redirect" suffix. Instead, the hyperlinks pointed directly to the classic UI. Users who clicked these links did not get the Blue Ocean UI, even if they had logged in to Jenkins and configured "Notification URL".
DisplayURLProvider.DisplayURLProviderImpl (source) overrides methods to append DISPLAY_POSTFIX = "display/redirect", and classes like RunDisplayAction (source) then call AbstractDisplayAction.lookupProvider(StaplerRequest) (source) to choose the provider that selects the redirect URL. But if the jenkins.displayurl.provider property points to org.jenkinsci.plugins.displayurlapi.ClassicDisplayURLProvider (source), then DisplayURLProvider.DisplayURLProviderImpl is not used and does not get a chance to add DISPLAY_POSTFIX.
Requested changes
Include "display/redirect" in URLs even if the default display URL provider has been configured with the system property or with the environment variable.
When a request comes to the URL with "display/redirect", and the user has not configured "Notification URL", use the default from the system property or the environment variable. This part may already work that way.
Could also define a separate system property, to keep the current behavior available. I don't know whether anyone really wants it, though.
- is caused by
-
-
- Resolved
-
- relates to
-
JENKINS-41578 Add a system property/environment variable to specify default display url impl
-
- Resolved
-
- links to
[JENKINS-69006] Impossible to configure default display URL provider but keep redirection (regression in 2.3.4)
Kalle Niemitalo
created issue -
Kalle Niemitalo
added a comment - Huh, DisplayURLProvider.get() actually used to return DisplayURLProviderImpl.INSTANCE always, but that was changed in https://github.com/jenkinsci/display-url-api-plugin/pull/42 to fix JENKINS-64119. That makes this a regression… I'll switch the type of this issue from Improvement to Bug.
Kalle Niemitalo
added a comment - Huh, DisplayURLProvider.get() actually used to return DisplayURLProviderImpl.INSTANCE always, but that was changed in https://github.com/jenkinsci/display-url-api-plugin/pull/42 to fix JENKINS-64119 . That makes this a regression… I'll switch the type of this issue from Improvement to Bug. Kalle Niemitalo
made changes -
| Issue Type | Original: Improvement [ 4 ] | New: Bug [ 1 ] |
Kalle Niemitalo
made changes -
| Link |
New:
This issue is caused by |
Kalle Niemitalo
made changes -
| Summary | Original: Configure default display URL provider but keep redirection | New: Impossible to configure default display URL provider but keep redirection |
Basil Crow
made changes -
| Labels | New: regression |
Basil Crow
made changes -
| Assignee | Original: James Dumay [ jamesdumay ] |
Basil Crow
made changes -
| Summary | Original: Impossible to configure default display URL provider but keep redirection | New: Impossible to configure default display URL provider but keep redirection (regression in 2.3.4) |
Kalle Niemitalo
added a comment - A simple revert of https://github.com/jenkinsci/display-url-api-plugin/pull/42 merge commit d25900536fb87e239c658f2576866d08ce85436d results in conflicts and would not be OK anyway:
- pom.xml:
- The PR removed <tag>display-url-api-2.3.3</tag>. Do not revert.
- src/main/java/org/jenkinsci/plugins/displayurlapi/DisplayURLProvider.java:
- DisplayURLProvider.get(): The PR made this call getPreferredProvider(). Revert.
- DisplayURLProvider.getRoot(): The PR made this call Jenkins.get() rather than the deprecated Jenkins.getInstance(). Do not revert.
- DisplayURLProvider.getPreferredProvider(): The PR made this check also the per-user setting rather than only the per-controller setting. Not sure whether to revert. This is called from DisplayURLProvider.lookupProvider() (which checks the per-user setting on its own), from DisplayURLProvider.get() (in which the call should be reverted), and from DisplayURLProvider.getDefault() (which is called from AbstractDisplayAction.lookupProvider(), which checks the per-user setting on its own, and from blueocean-display-url-plugin). I think it would be nicer to revert this, just to reduce the redundant checking of per-user settings. GitHub code search does not show this being called by other plugins.
- DisplayURLProvider.getUserPreferredProviderProperty(): The PR added this method. Do not revert, because other plugins may now be using it (although I don't see why any would).
- src/main/java/org/jenkinsci/plugins/displayurlapi/actions/AbstractDisplayAction.java:
- AbstractDisplayAction.getUserPreferredProviderProperty(): The PR deprecated this method and made it call DisplayURLProvider.getUserPreferredProviderProperty(). If the latter method is not removed or deprecated, then this change should not be reverted either.
- src/test/java/org/jenkinsci/plugins/displayurlapi/DisplayURLProviderTest.java:
- DisplayURLProviderTest.TestSysPropDisplayURLProvider: The PR added this class. Do not revert, because it will be needed in the rewritten urlsWithSysPropProvider().
- DisplayURLProviderTest.urlsWithSysPropProvider(): The PR added this test method. Rewrite to assert that
JENKINS-69006is fixed. - DisplayURLProviderTest.TestUserDisplayURLProvider: The PR added this class. Do not revert, because it will be needed in the rewritten urlsWithUserDefinedProvider().
- DisplayURLProviderTest.urlsWithUserDefinedProvider(): The PR added this test method. Rewrite to assert that
JENKINS-69006is fixed. - DisplayURLProviderTest.decoration(): The PR made this use try-with-resources syntax. Do not revert.
Kalle Niemitalo
added a comment - A simple revert of https://github.com/jenkinsci/display-url-api-plugin/pull/42 merge commit d25900536fb87e239c658f2576866d08ce85436d results in conflicts and would not be OK anyway:
pom.xml:
The PR removed <tag>display-url-api-2.3.3</tag> . Do not revert.
src/main/java/org/jenkinsci/plugins/displayurlapi/DisplayURLProvider.java:
DisplayURLProvider.get(): The PR made this call getPreferredProvider(). Revert.
DisplayURLProvider.getRoot(): The PR made this call Jenkins.get() rather than the deprecated Jenkins.getInstance() . Do not revert.
DisplayURLProvider.getPreferredProvider(): The PR made this check also the per-user setting rather than only the per-controller setting. Not sure whether to revert. This is called from DisplayURLProvider.lookupProvider() (which checks the per-user setting on its own), from DisplayURLProvider.get() (in which the call should be reverted), and from DisplayURLProvider.getDefault() (which is called from AbstractDisplayAction.lookupProvider(), which checks the per-user setting on its own, and from blueocean-display-url-plugin). I think it would be nicer to revert this, just to reduce the redundant checking of per-user settings. GitHub code search does not show this being called by other plugins.
DisplayURLProvider.getUserPreferredProviderProperty(): The PR added this method. Do not revert , because other plugins may now be using it (although I don't see why any would).
src/main/java/org/jenkinsci/plugins/displayurlapi/actions/AbstractDisplayAction.java:
AbstractDisplayAction.getUserPreferredProviderProperty(): The PR deprecated this method and made it call DisplayURLProvider.getUserPreferredProviderProperty(). If the latter method is not removed or deprecated, then this change should not be reverted either.
src/test/java/org/jenkinsci/plugins/displayurlapi/DisplayURLProviderTest.java:
DisplayURLProviderTest.TestSysPropDisplayURLProvider: The PR added this class. Do not revert , because it will be needed in the rewritten urlsWithSysPropProvider().
DisplayURLProviderTest.urlsWithSysPropProvider(): The PR added this test method. Rewrite to assert that JENKINS-69006 is fixed.
DisplayURLProviderTest.TestUserDisplayURLProvider: The PR added this class. Do not revert , because it will be needed in the rewritten urlsWithUserDefinedProvider().
DisplayURLProviderTest.urlsWithUserDefinedProvider(): The PR added this test method. Rewrite to assert that JENKINS-69006 is fixed.
DisplayURLProviderTest.decoration(): The PR made this use try-with-resources syntax. Do not revert.
List of known DisplayURLProvider implementations: https://www.jenkins.io/doc/developer/extensions/display-url-api/#displayurlprovider
Both DisplayURLProvider.DisplayURLProviderImpl and PipelineGraphDisplayURLProvider extend ClassicDisplayURLProvider. If this feature were implemented by modifying ClassicDisplayURLProvider, it might require changes in those as well.
I think it would be better to make DisplayURLProvider.get() skip getPreferredProvider() and just always return DisplayURLProviderImpl.INSTANCE, whose methods would then append "display/redirect". In contrast, DisplayURLProvider.getDefault(), DisplayURLProvider.getPreferredProvider(), and AbstractDisplayAction.lookupProvider would still respect the per-user or global default.
GitHub search for org:jenkinsci DisplayURLProvider getDefault shows blueocean-display-url-plugin calling DisplayURLProvider.getDefault() rather than DisplayURLProvider.get(). I guess that may be OK, if Blue Ocean returns that URL to the browser right away, instead of sending the URL to a service that can store the URL persistently.