feat: display value selector #85

rolaca11 · 2021-08-12T20:09:30Z

Description

implements #81

Changes

added a new field to Item class, replaced string list of options to Item list. In the end, the HTML Option tag has the value expression in its value property and the string displayed is the displayValue field.

Issues

Didn't test for xPath. Code's probably terrible, first time working with Jenkins plugins and their dependencies

…t display value to the one included in env variables

h1dden-da3m0n

Thank you for your contribution here, much appreciated!
Left a few comments on the things that I would like to discuss before giving it a green check ;)

src/main/resources/io/jenkins/plugins/restlistparam/RestListParameterDefinition/config.jelly

src/main/java/io/jenkins/plugins/restlistparam/model/Item.java

src/main/java/io/jenkins/plugins/restlistparam/RestListParameterDefinition.java

src/main/java/io/jenkins/plugins/restlistparam/logic/ValueResolver.java

h1dden-da3m0n · 2021-08-13T15:39:43Z

src/main/resources/io/jenkins/plugins/restlistparam/RestListParameterDefinition/index.jelly

@@ -20,7 +20,7 @@
      <select id="${id}-select" class="setting-input" name="value">
        <j:forEach var="value" items="${it.values}" varStatus="loop">
          <f:option value="${value}" selected="${value.equals(it.defaultValue)}">
-            ${value}
+            ${value.displayValue}


while something fancy like select element templating is a option with Select2 I guess this works for now

src/main/resources/io/jenkins/plugins/restlistparam/RestListParameterDefinition/index.jelly

src/test/java/io/jenkins/plugins/restlistparam/RestValueServiceTest.java

rolaca11 · 2021-08-15T12:06:39Z

lmk if I missed something, or if you have any more comments

h1dden-da3m0n · 2021-08-15T16:43:09Z

Thank you for the update I will give this another review either today or tomorrow!

h1dden-da3m0n

So far so good, sorry for not noticing the re-run issue lat time around. But other than that I didn't notice any problem besides the other nitpick.

src/main/resources/io/jenkins/plugins/restlistparam/RestListParameterDefinition/index.jelly

src/main/resources/io/jenkins/plugins/restlistparam/RestListParameterDefinition/config.jelly

rolaca11 · 2021-09-04T14:56:13Z

Sorry for the late update, took a long vacation and it's hard to get back on the grind :D

h1dden-da3m0n · 2021-09-04T15:38:35Z

Welcome back! No worries, I am currently fighting with the Jenkins Infra, cause it refuses to publish 1.4.2, but I will have a look over your changes once that is dealt with.

edit1: just found the reason why I cant release, but am non the wiser https://groups.google.com/g/jenkinsci-dev/c/1-46eS41y6I 😞

edit2: now I know why https://www.jenkins.io/blog/2021/09/04/wiki-attacked and until that situation has been dealt with I am stuck with my local copy waiting to release 1.4.2 to the Jenkins repo.

# Conflicts: # src/main/java/io/jenkins/plugins/restlistparam/RestListParameterDefinition.java

rolaca11 · 2021-09-05T12:20:04Z

edit2: now I know why https://www.jenkins.io/blog/2021/09/04/wiki-attacked and until that situation has been dealt with I am stuck with my local copy waiting to release 1.4.2 to the Jenkins repo.

That sounds terrible, hope they can figure it out soon.

PS.: In the meantime, I have a proposal for the server paging problem. It turns out to be kinda difficult, but I do have an immature version that works with nexus flawlessly. It can be found in my fork on the feature/82 branch. I didn't create a PR for it because that feature is built on this PR and also it does need to be enhanced a bit, and I wanted to get your input on the matter first.

h1dden-da3m0n · 2021-09-05T15:27:32Z

PS.: In the meantime, I have a proposal for the server paging problem. It turns out to be kinda difficult, but I do have an immature version that works with nexus flawlessly. It can be found in my fork on the feature/82 branch. I didn't create a PR for it because that feature is built on this PR and also it does need to be enhanced a bit, and I wanted to get your input on the matter first.

NGL I spotted that yesterday, and thank you for exploring that feature!
Are you okay with waiting for my input until this PR is in? It would make things easier IMHO, as we could utilize the review process to exchange comments.

So far, from my glance over it I think your changes look like they follow a direction I wanted to explore with that feature, but I will refrain further comment as I really only glanced over the changes.

rolaca11 · 2021-09-05T17:22:40Z

Of course, I'm okay with that. I just wanted to bring this to your attention. It already does what I need it to do, but I'd like it to be something more than just a selfish endevour

h1dden-da3m0n · 2021-09-08T19:25:53Z

was able to release 1.4.2 today. I will be reviewing this PR on the weekend 😉

h1dden-da3m0n · 2021-09-11T17:37:35Z

Hey @rolaca11, I now managed to look at your changes here again. I ran into a couple of minor things that I would like to commit back to you here and discuss if you are alright with that?
Alongside, those minor things I might have stumbled onto something more problematic though, that I need some more time to pinpoint if we can fix. To be precise its related to how this changes the createValue functions and wether the values list is empty or not.

rolaca11 · 2021-09-12T09:55:24Z

Of course, hit me. It's your codebase, your calls :D

h1dden-da3m0n · 2021-09-12T13:52:41Z

Of course, hit me. It's your codebase, your calls :D

I just prefer asking before writing to pls branches ;) this now 'works' from a to z (Re-Build included, sorry for focusing on that one xD)

However, I ran into an interesting side effect of the change to rely on the values list being set within the createValue functions, that can lead to a IndexOutOfBounds. Still trying to tracking that down, but seems for now to be only for 'legacy' runs to give of the behaviour 🤔

rolaca11 · 2021-09-25T15:22:20Z

However, I ran into an interesting side effect of the change to rely on the values list being set within the createValue functions, that can lead to a IndexOutOfBounds. Still trying to tracking that down, but seems for now to be only for 'legacy' runs to give of the behaviour thinking

I don't think I get what you're saying, does it cause problems when upgrading to the new version of this plugin, or some legacy Jenkins versions?

h1dden-da3m0n · 2021-09-25T21:28:12Z

Sorry, I hoped to have properly tested this over the past week but life got in my way. Anyway, I was referring to upgrading from prior version of this plugin. I will continue to be busy for the next couple of days unfortunately, but I will try to get back to this as soon as possible

h1dden-da3m0n · 2021-10-04T18:31:21Z

Okay, sorry again for taking ages with your PR. I now have figured out what it is that breaks, but have not yet found a suitable fix for it. (at least not one I am comfortable with)

It is save to say that my originally proposed idea of referencing just the index in the values list is not a good idea when trying to rebuild a job. so the value has to be passed directly, however, I really have a problem with dumping that json directly into the value property of the select element 🤔

h1dden-da3m0n · 2021-10-17T18:55:30Z

Found some time this weekend to look a bit further into this and here my findings:

The odd behaviour with Re-Runs I described in an earlier comment is an up till now unnoticed bug irrelevant to your PR
I think there might be a better solution for how to store the display value, but I am currently just testing what I have thought up
once I have that tested out and decided I will push some final changes and then merge your PR

Thank you for your contribution and patience with me!

edit: while diving into this a bit more I came very close ripping out XML entirely from the plugin cause WHO IN THEIR RIGHT MIND STILL USES XML AS THEIR REST RETURN VALUE IN 2021!? (it is save to assume I got a bit tilted testing for XML)

rolaca11 · 2021-10-17T19:40:38Z

Nice, thanks for your time and help and all that.

I'm happy to help you with something in connection with the PR if you have anything

h1dden-da3m0n · 2021-10-18T21:32:28Z

@rolaca11 I just pushed my 'refactor' for the display value logic. If you could take one further look over the changes and give me feedback or fix any glaring issue you may find, I would really appreciate it!

You may also notice that I added a notice that XML is not supported, as neither your nor my code is able to handle it. It drove me crazy yesterday and lead to decision that lead to #99.

edit

I should mention that your solution would have allowed for the sub expression to work, in a way, however the output of a XML sub object with or without a display expression would look something like this:

'Input'

<row>
  <name>v10.6.4</name>
  <zipball_url>https://api.github.com/repos/jellyfin/jellyfin/zipball/v10.6.4</zipball_url>
  <tarball_url>https://api.github.com/repos/jellyfin/jellyfin/tarball/v10.6.4</tarball_url>
  <commit>
    <sha>b49cd1d3017f23fc75703829ac2ea1d45d8a4881</sha>
    <url>https://api.github.com/repos/jellyfin/jellyfin/commits/b49cd1d3017f23fc75703829ac2ea1d45d8a4881</url>
  </commit>
  <node_id>MDM6UmVmMTYxMDEyMDE5OnJlZnMvdGFncy92MTAuNi40</node_id>
</row>

in a pipeline the parameter value would look like this:

  v10.6.4
  https://api.github.com/repos/jellyfin/jellyfin/zipball/v10.6.4
  https://api.github.com/repos/jellyfin/jellyfin/tarball/v10.6.4
    b49cd1d3017f23fc75703829ac2ea1d45d8a4881
    https://api.github.com/repos/jellyfin/jellyfin/commits/b49cd1d3017f23fc75703829ac2ea1d45d8a4881
  MDM6UmVmMTYxMDEyMDE5OnJlZnMvdGFncy92MTAuNi40

(because of the behaviour that getTextContent() has and the lack of an alternative)

rolaca11 · 2021-10-20T19:59:35Z

oo, now I see why this isn't compatible at all with XML stuff... nice catch. I see no obvious issues with your latest commits, so if you think it can go in, I think it can go in as well.

src/main/resources/io/jenkins/plugins/restlistparam/RestListParameterValue/value.jelly

h1dden-da3m0n

This LGTM now. Thank you again for your contribution rolaca!

jenkinsci#81 add a displayExpression property, that allows a differen…

55ac61a

…t display value to the one included in env variables

rolaca11 requested a review from a team as a code owner August 12, 2021 20:09

jenkinsci#81 fix test configuration

89b5075

h1dden-da3m0n added feature This PR introduces a new feature breaking This PR introduces breaking changes labels Aug 12, 2021

h1dden-da3m0n reviewed Aug 13, 2021

View reviewed changes

jenkinsci#81 enhancements based on review comments

e396814

h1dden-da3m0n self-requested a review August 15, 2021 16:43

h1dden-da3m0n reviewed Aug 16, 2021

View reviewed changes

src/main/resources/io/jenkins/plugins/restlistparam/RestListParameterDefinition/index.jelly Outdated Show resolved Hide resolved

src/main/resources/io/jenkins/plugins/restlistparam/RestListParameterDefinition/config.jelly Outdated Show resolved Hide resolved

jenkinsci#81 revert select option values to be the actual value

283943e

Merge remote-tracking branch 'origin/main' into feature/81

229f523

# Conflicts: # src/main/java/io/jenkins/plugins/restlistparam/RestListParameterDefinition.java

fix: parameter rebuild behaviour

fa479db

h1dden-da3m0n added 2 commits October 18, 2021 23:26

refactor display value logic

8a068e2

bump version

ba14795

h1dden-da3m0n mentioned this pull request Oct 18, 2021

Whats Next? #99

Open

h1dden-da3m0n added the minor Merging this PR leads to a minor version increment label Oct 18, 2021

h1dden-da3m0n reviewed Oct 21, 2021

View reviewed changes

src/main/resources/io/jenkins/plugins/restlistparam/RestListParameterValue/value.jelly Outdated Show resolved Hide resolved

remove no longer working tooltip

acfd543

h1dden-da3m0n approved these changes Oct 21, 2021

View reviewed changes

h1dden-da3m0n merged commit 64dac20 into jenkinsci:main Oct 21, 2021

h1dden-da3m0n changed the title ~~#81 add a displayExpression property, that allows a different display value to the one included in env variables~~ feat: display value selector Oct 21, 2021

h1dden-da3m0n mentioned this pull request Oct 23, 2021

docu: add display value expression docu #100

Merged

1 task

bugreportsonly mentioned this pull request Nov 28, 2022

[BUG] RestListParameterValue Not Exported To Remote Access API #174

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: display value selector #85

feat: display value selector #85

rolaca11 commented Aug 12, 2021 •

edited

Loading

h1dden-da3m0n left a comment

h1dden-da3m0n Aug 13, 2021

rolaca11 commented Aug 15, 2021

h1dden-da3m0n commented Aug 15, 2021

h1dden-da3m0n left a comment

rolaca11 commented Sep 4, 2021

h1dden-da3m0n commented Sep 4, 2021 •

edited

Loading

rolaca11 commented Sep 5, 2021 •

edited

Loading

h1dden-da3m0n commented Sep 5, 2021

rolaca11 commented Sep 5, 2021

h1dden-da3m0n commented Sep 8, 2021

h1dden-da3m0n commented Sep 11, 2021

rolaca11 commented Sep 12, 2021

h1dden-da3m0n commented Sep 12, 2021

rolaca11 commented Sep 25, 2021

h1dden-da3m0n commented Sep 25, 2021

h1dden-da3m0n commented Oct 4, 2021

h1dden-da3m0n commented Oct 17, 2021 •

edited

Loading

rolaca11 commented Oct 17, 2021

h1dden-da3m0n commented Oct 18, 2021 •

edited

Loading

rolaca11 commented Oct 20, 2021

h1dden-da3m0n left a comment

feat: display value selector #85

feat: display value selector #85

Conversation

rolaca11 commented Aug 12, 2021 • edited Loading

Description

Changes

Issues

h1dden-da3m0n left a comment

Choose a reason for hiding this comment

h1dden-da3m0n Aug 13, 2021

Choose a reason for hiding this comment

rolaca11 commented Aug 15, 2021

h1dden-da3m0n commented Aug 15, 2021

h1dden-da3m0n left a comment

Choose a reason for hiding this comment

rolaca11 commented Sep 4, 2021

h1dden-da3m0n commented Sep 4, 2021 • edited Loading

rolaca11 commented Sep 5, 2021 • edited Loading

h1dden-da3m0n commented Sep 5, 2021

rolaca11 commented Sep 5, 2021

h1dden-da3m0n commented Sep 8, 2021

h1dden-da3m0n commented Sep 11, 2021

rolaca11 commented Sep 12, 2021

h1dden-da3m0n commented Sep 12, 2021

rolaca11 commented Sep 25, 2021

h1dden-da3m0n commented Sep 25, 2021

h1dden-da3m0n commented Oct 4, 2021

h1dden-da3m0n commented Oct 17, 2021 • edited Loading

rolaca11 commented Oct 17, 2021

h1dden-da3m0n commented Oct 18, 2021 • edited Loading

edit

rolaca11 commented Oct 20, 2021

h1dden-da3m0n left a comment

Choose a reason for hiding this comment

rolaca11 commented Aug 12, 2021 •

edited

Loading

h1dden-da3m0n commented Sep 4, 2021 •

edited

Loading

rolaca11 commented Sep 5, 2021 •

edited

Loading

h1dden-da3m0n commented Oct 17, 2021 •

edited

Loading

h1dden-da3m0n commented Oct 18, 2021 •

edited

Loading