jenkinsci / sidebar-link-plugin

Jenkins sidebar-link plugin
https://plugins.jenkins.io/sidebar-link/
MIT License
26 stars 43 forks source link

Implement from reference #31

Closed ClundXIII closed 2 years ago

ClundXIII commented 2 years ago

Option for adding ?from=<path> or &from=path to URI.

This is useful if you want to link to an external tool that uses the reference for navigational reasons.

Since this is the first time touching any code related to Jenkins, there are some issues that I could not solve (The reason I am submitting this as a draft):

ClundXIII commented 2 years ago

Also it might be better to parse the URL, add a param and then generate it again (instead of using `if [...] .contains("?")). I am hesitant to add more dependencies so if someone could point me to the usual jenkins library to do that I will adapt the code.

damianszczepanik commented 2 years ago

@ClundXIII are you working on this?

ClundXIII commented 2 years ago

@ClundXIII are you working on this?

I kinda paused, since I ran into a few problems.

Can you point me into the right direction? How can I make the settings persist and if and what library I should use for URL verification?

KalleOlaviNiemitalo commented 2 years ago

It might be better to make the referrer URL just another variable that could then be used via https://github.com/jenkinsci/sidebar-link-plugin/issues/23. That way the from parameter name would be part of the user-configurable template.

KalleOlaviNiemitalo commented 2 years ago

If you include a URL as a query parameter in another URL then it should be encoded, so that & signs in the inner URL can be distinguished from parameter separators in the outer URL. I suspect that automatic URL-encoding of values might be difficult to implement if the Sidebar Link plugin used the Token Macro plugin to expand the values.

KalleOlaviNiemitalo commented 2 years ago

An implementation based on RFC 6570 URI templates would have the advantage that the user could choose whether the parameters go into the query component or into the fragment component, even when the URI template also has other stuff in the fragment component.

ClundXIII commented 2 years ago

@KalleOlaviNiemitalo @damianszczepanik

I am open to your suggestion, however I am not sure what you mean exactly. Something analogue to www.example.com/job/$job/job/$branch/$BUILD_NUMBER or www.example.com/?reference=job/$job/job/$branch/$BUILD_NUMBER ?

Is rfc6570 already implemented somewhere in Jenkins?

KalleOlaviNiemitalo commented 2 years ago

I don't think rfc6570 has been already implemented in Jenkins. I looked for Maven packages that implement it, and there are some, but it might be better for supply-chain security if the URI template syntax were implemented from scratch in this plugin. Level 1 templates look pretty simole to implement.

KalleOlaviNiemitalo commented 2 years ago

The syntax would be https://www.example.com/job/{job}/job/{branch}/{BUILD_NUMBER} or https://www.example.com/?reference=job/{job}/job/{branch}/{BUILD_NUMBER}.

If level 4 templates were supported, then you could even use https://www.example.com/{?job,branch,BUILD_NUMBER} and have it expand to https://www.example.com/?job=sample&branch=master on a job page where BUILD_NUMBER is not available.

If {branch} were supported, I think the value should come from SCMHead.getName() or Branch.getName() and then encoded during URI template processing. It should not come from Branch.getEncodedName() nor Item.getName().

There could be a template parameter for the absolute URI of the Job or Run for which the link is displayed. That would use DisplayURLProvider.

If you want a template parameter for the Item.getFullName() of the MultiBranchProject that contains the branch for which the link is displayed, then I think that should be called something other than {job}.

ClundXIII commented 2 years ago

My comment was meant as an example of something that works from inside a groovy script ...

Right now I have no Idea what to do. I asked for advice, got none. I definitely don't have the time to implement the rfc6570 you mentioned.

Is there any chance that this feature will be accepted into upstream or should I just close the PR?