Add support for custom defined labels (verdict categories beside Verified & Code-Review) #393
Conversation
|
Hi! Anyone up for some reviewing? |
| private Integer gerritBuildUnstableCodeReviewValue = null; | ||
| private Integer gerritBuildNotBuiltCodeReviewValue = null; | ||
| @Deprecated | ||
| public Integer gerritBuildStartedVerifiedValue = null; |
There was a problem hiding this comment.
why make these public if they are deprecated?
There was a problem hiding this comment.
I don't quite recall why I made those deprecated since they need to be used, so I removed the 'Deprecated' annotations
src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/config/Config.java
Outdated
Show resolved
Hide resolved
| @@ -378,24 +426,34 @@ public void setValues(JSONObject formData) { | |||
| "projectListFetchDelay", | |||
| DEFAULT_PROJECT_LIST_FETCH_DELAY); | |||
|
|
|||
| projectListRefreshInterval = formData.optInt( | |||
There was a problem hiding this comment.
projectListRefreshInterval and enableProjectAutoCompletion seems to have been removed and not replaced?
| cat.setDefaultBuildStartedReportingValue(getIntegerFromString((String) formData.get(cat.getVerdictValue() + "Started"))); | ||
| if (formData.has(cat.getVerdictValue() + "Unstable")) | ||
| cat.setDefaultBuildUnstableReportingValue(getIntegerFromString((String) formData.get(cat.getVerdictValue() + "Unstable"))); | ||
| if (formData.has(cat.getVerdictValue() + "Not Built")) |
There was a problem hiding this comment.
Whitespaces in json keys makes me a bit nervous, though probably fully legal :)
| assertDefaultCategories(); | ||
| //cover the migration of default gerrit reporting values | ||
| for (VerdictCategory cat : categories) { | ||
| if (formData.has(cat.getVerdictValue() + "Successful")) |
There was a problem hiding this comment.
Isn't this code already in VerdictCategory.fromJSON?
There was a problem hiding this comment.
Not quite, in VerdictCategory we create a new object with default reporting values, whereas this section is to cover the case when we switch from a config that doesn't support multiple labels to one that does.
| @@ -413,6 +471,44 @@ public void setValues(JSONObject formData) { | |||
| replicationConfig = ReplicationConfig.createReplicationConfigFromJSON(formData); | |||
| } | |||
|
|
|||
| private void initializeGerritCommands() { | |||
| List<String> stringLabels = new ArrayList<String>(Arrays.asList("verified", "code-review")); | |||
There was a problem hiding this comment.
Slight efficiency if this was a Set<String> since verified and code-review could be added twice, and this list is mostly used for lookup, so a HashSet would be quicker at those. But ordering is not preserved if that is important?.
src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/config/Config.java
Show resolved
Hide resolved
| gerritVerifiedCmdBuildNotBuilt = gerritVerifiedCmdBuildNotBuilt.contains(commandAddition) ? gerritVerifiedCmdBuildNotBuilt : | ||
| (gerritVerifiedCmdBuildNotBuilt += commandAddition); | ||
| } | ||
| } |
There was a problem hiding this comment.
It is probably simpler, or a bit more straight forward at least, if all the templated --code-review <CODE-REVIEW> ... was replaced with just a or similar that the ParameterExpander would later expand to all those parameters.
The readResolve can do the replacement in any existing configured commands.
wdyt?
There was a problem hiding this comment.
I remember though that there are some searching going on in the rest commands to extract the data out of the commands that might need some changes as well if you haven't already.
There was a problem hiding this comment.
At that time, this was the "least invasive" way to add the commands for additional labels. But maybe it would be an improvement for a future commit?
There was a problem hiding this comment.
Well this change is quite invasive as it is, so slightly more invasive wouldn't do much difference I think :)
|
Sorry, a bit too big for me to go though atm. @TWestling didn't you plan on doing this as well? Care to review? |
|
Thank you @rsandell! Bear in mind that some unit tests still need to be fixed in order to get the CI success (free time outside of work was pretty tight). This PR is definitely (maybe too?) big but this is due to how everything is hardcoded around code-review & verified. I've refactored as much of the base functionality as possible without having to rewrite the whole plugin and I will say that we have used this version in production environment for well over 1.5 years without any issues whatsoever. |
|
Sorry about completely missing this. Yes @rsandell I started implementing this but couldn't prioritize it enough. |
| * Converter from String to Integer. | ||
| * | ||
| * @param str the String to be parsed as Integer. | ||
| * @return the resulting Integer |
There was a problem hiding this comment.
"or null in case of non successful parse"
| ? gerritVerifiedCmdBuildNotBuilt : (gerritVerifiedCmdBuildNotBuilt += commandAddition); | ||
| } | ||
| } | ||
| String gerritCmdRegex = "(--([\\S]+) (<[\\S]+>)){1}"; |
There was a problem hiding this comment.
Is the {1} repetition needed really needed here?
| Pattern pattern = Pattern.compile(gerritCmdRegex); | ||
| Matcher matcher = pattern.matcher(gerritVerifiedCmdBuildStarted); | ||
| while (matcher.find()) { | ||
| if (!stringLabels.contains(matcher.group(2))) { |
There was a problem hiding this comment.
Perhaps I'm misreading the code, but group 1 contains the verdictValue.lowercase and group 2 the uppercase version. Will this ever match?
|
Do we have a plan going forward on this? It's a great change, but so huge. And now we are seeing merge conflicts. @GeorgeCimpoies: Are you still interested in getting this merged? @rsandell & @TWestling : Thoughts on this one? Can we just have the few comments fixed and release it as a beta? Otherwise it seems we will need to close it. |
|
I would like to get it merged as well. It is a well needed feature. Unfortunately I won't have time to review something this huge during the little time I have alotted for reviews like this. So I was hoping to trust you guys to say if it is good or not ;) |
|
Yes, definitely interested in merging this, I will try to rebase and answer the pending comments. Also, we have used it in production environment since it was developed: Jenkins instance with around 2500 jobs, Gerrit instance with around 1100 projects) with various configurations and workflows (e.g. keeping Verified label for patchset jobs and adding new labels such as Validated for more extensive tests, before merging/after Code-Review+2) without any issues so far. |
|
If so, I propose that after @GeorgeCimpoies update with the latest fixes, that we merges his code and issue an beta release for testing. |
|
We have a need for this fix as well. Any way we can help this along? |
|
Anyone still working on this or know of a good workaround? |
Refactoring mainly impacts the classes:
Config.java
GerritTrigger.java
ParameterExpander.java
It includes:
Ability to define & add custom labels from the Gerrit Server configuration page -> it will also dynamically add the custom label intro the gerrit commands for build successful, failed, etc.
Ability to modify reporting values for each job, from the job configuration page (Gerrit Trigger -> Advanced section)
Refactoring of the gerrit notification process defined in ParameterExpander, to support the addition of custom labels
Fixed unit tests
No API changes, no UI changes besides a small section in the job configuration page (the addition of custom labels defined in the server config page to the job config page)