Skip to content

[WiP] - Store values from multi-level multi select list#9

Closed
lkisac wants to merge 0 commit into
jenkinsci:masterfrom
lkisac:master
Closed

[WiP] - Store values from multi-level multi select list#9
lkisac wants to merge 0 commit into
jenkinsci:masterfrom
lkisac:master

Conversation

@lkisac

@lkisac lkisac commented Jan 18, 2015

Copy link
Copy Markdown

I added a feature to store multiple values for the multi level select list.

It stores each selection and returns a string. This is an example with 3 levels for the multi select list:
{1:dropdown1value,dropdown2value,dropdown3value:2:dropdown1value,dropdown2value,dropdown3value:3:dropdown1value,dropdown2value,dropdown3value:} etc.

The Extended Choice Parameter field's name holds the string value.
Example (Jenkins Shell script):
echo ${Multi_Level_List}

I have a perl script that parses this string and build a hash containing all multi select values.

sub ParseMultiLevelString {
my $multiLevelString = shift;
my %hash;
my $eachMulti = 1; # i.e. { => 1:dropdownval1....}

my ( $dropdown1header, $dropdown2header, $dropdown3header ) = ( "", "", "" );

while ( $multiLevelString =~ m/[$eachMulti]{1}:([^:]+),([^:]+),([^:]+):[0-9\}]{1}/ ) {
       # store into hash
    $hash{$eachMulti}{'dropdownOne'}     = $1;
    $hash{$eachMulti}{'dropdownTwo'}     = $2;
    $hash{$eachMulti}{'dropdownThree'}  = $3;

    $eachHost++;
    }
return ( \%buildSetupHash, $eachHost );

}

@oleg-nenashev

Copy link
Copy Markdown
Member

It's hard to review changes due to the multiple formatting changes.
Move them to a separate commit at least

@lkisac lkisac changed the title Store multiple values from multi-level multi select list Store values from multi-level multi select list Jan 18, 2015
@jenkinsadmin

Copy link
Copy Markdown
Member

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@lkisac

lkisac commented Jan 19, 2015

Copy link
Copy Markdown
Author

This commit shows the changes from the original without all the formatted lines
lkisac@84c44d0

@oleg-nenashev

Copy link
Copy Markdown
Member

I'm not skilled enough in JavaScript to review the code, but the entire Java part should be reworked

@oleg-nenashev oleg-nenashev changed the title Store values from multi-level multi select list [WiP] - Store values from multi-level multi select list Jan 19, 2015
@lkisac

lkisac commented Jan 20, 2015

Copy link
Copy Markdown
Author

Thanks for the clear feedback. Based off your review, I made the necessary changes. The initial extension I did was a quick fix so we could use it right away and I noticed other people also inquiring about how to store all values from the multi-level select list.

One thing I was unable to get working was the logger (java.util.logger.Logger), it doesn't seem to be displaying in the console as System IO was. Since it's only for debugging purposes, I have another commit without the debugging lines.

I've been using this modified plugin for several months and it's been working well so far with the various plugins on our Jenkins instance.

@oleg-nenashev

Copy link
Copy Markdown
Member

@lkisac

  • The concurrency issue for multiLevelColumns, allCols and strValue has not been solved
  • There should be only one DataBoundConstructor. I'm not sure if you use the proper one
  • IMO such detailed logging is not required, but it's up to the plugin's owner

I would recommend to merge you commits into a single one in order to review the code again.
Unit tests for the feature would be useful as well.

@lkisac

lkisac commented Jan 22, 2015

Copy link
Copy Markdown
Author

I created a new branch from master before the changes I did (91339d6). I edited the files and I was going to use squash or rebase to merge commits into one, doing something like git reset --soft HEAD~7 but instead I used git reset --hard 91339d65f064c21472f4402be4a892c78a9bd4d6 to remove those commits and merge that latest branch, but didn't realize it would close this pull request. I do have my latest commit showing in my master, so I'm not sure why it won't appear here and run the test.

Should I open a new pull request?

I don't have Unit Tests written yet, I'm still getting acquainted with JUnit and HTMLUnit. However, I tried various scenarios with the plugin on my local Jenkins and found an issue with the way the values were being stored, which I've fixed.

The addition I made is fairly simple, so I don't think it should break anything else, but I have another branch where I'll work on Unit Tests for this.

@oleg-nenashev

Copy link
Copy Markdown
Member

@lkisac
IMO It's hard to break the plugin in the scope of the architecture. The current version really demands a re-work.

Should I open a new pull request?

Force push should resolve any issue

@lkisac

lkisac commented Jan 22, 2015

Copy link
Copy Markdown
Author

It says everything is up-to-date.

This is my latest commit b7a5a76, but it's not showing here for me to click the 'Reopen and comment' button.

@lkisac

lkisac commented Jan 22, 2015

Copy link
Copy Markdown
Author

I read that it's not possible to re-open a merged pull request.

Would it be ok to go ahead and open a new pull request from my branch?

@oleg-nenashev

Copy link
Copy Markdown
Member

Your PR has not been merged. You just closed it.

BTW feel free to open a new one if it is more convenient for you. Just don't forget to reference this PR

@lkisac

lkisac commented Jan 22, 2015

Copy link
Copy Markdown
Author

Ah right, the PR has not been merged to jenkinsci-master yet, no. I was thinking of my merge from branch to lkisac-master.

Unfortunately it's not showing that merge here as it was during my previous commits. I think the git reset --hard 91339d65f064c21472f4402be4a892c78a9bd4d6 I did to delete those old commits closed the PR and now it won't recognize any new commits after being closed. git reset --soft or git merge --squash might've been better to use.

At any rate, I guess it would be easier to open a new one and reference this PR.
I'll go ahead and do that. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants