JENKINS-25223: Allow executing scripts in JobProperty.prebuild phase #36
Conversation
|
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
|
IMHO, it's preferable to merge the functionality into https://github.com/jenkinsci/envinject-plugin/blob/master/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectJobProperty.java (just to avoid user confusion and extra checkboxes) |
|
Well, the point was actually not to. If there is only one box it'll be either "before SCM" or "after SCM before doRun". There is no reason to not allow both but I agree it makes sense to work on the naming for the boxes. Scenario could be:
|
|
Isn't it time the UI was changed to something more like Git Plugin's Additional Behaviors, or how Email-ext changed the per-trigger recipients? It's getting ridiculous with the boxes. |
|
I'm not opposed to the change in the UI. Let me see how additional behaviors work in Git and how long it'll take to get it done. |
|
Just out of curiousity, what section of the job would you like the configuration to reside? I would vote for top-most Job Property section for all, i.e. for "before SCM", "after SCM, before doRun", "shortly after doRun" (not real labels). |
|
I'd like to get this feature merged. BTW, a combination of EnvInject build step and https://wiki.jenkins-ci.org/display/JENKINS/pre-scm-buildstep seems to provide this feature. |
|
|
||
| @SuppressWarnings("unused") | ||
| public EnvInjectJobPropertyInfo getInfo() | ||
| { |
There was a problem hiding this comment.
It would be great if you follow the plugin's codestyle. Brackets are being placed at the same line as the declaration in the plugin
There was a problem hiding this comment.
Thanks, I'll work on the style, standby.
Actually I don't think it does, as specific stated purpose is (emphasis added):
This allows to get a code tree sucked in and then configure things like JDK, Maven or any other build environment component based on information retrieved from the SCM. |
|
It would be great to have a 1 or 2 unit tests for the feature (just to be sure it works correctly when we apply new changes). It should not be a big deal, because it's possible to copy some bits from #58. Could you add such tests? |
|
I'll add the tests |
|
@oleg-nenashev Tests added, latest functionality synced up. Happy? 😄 |
|
@arcivanov Adding the PR to the next major release milestone. It would be great if you squash commits |
…d phase Creates the "prebuild" phase JobProperty section, allowing scripts to be executed after SCM is completed but before AbstractBuildExecution.doRun kicks in. Also fixes NPE in EnvInjectJobProperty where disabling a section and saving before applying causes NPE on save
arcivanov
force-pushed
the
JENKINS-25223
branch
from
c1fcac8 to
47f4781
Compare
|
@oleg-nenashev squashed, commit message updated to reflect fixed status |
|
@oleg-nenashev bump |
It definitely makes sense (as well as the rework of the internal layout). BTW it should be done as a general envinject-2.0 effort. I'm pretty OK to merge the PR now, but I would plan to rework it during the 2.0 rework and probably make the incompatible change. |
|
I agree that UI rework is out of scope for this PR - it was my intention to only introduce additional functionality. |
|
|
||
| if (onObject != null) { | ||
| EnvInjectPrebuildJobProperty envInjectJobProperty = new EnvInjectPrebuildJobProperty(); | ||
| EnvInjectJobPropertyInfo info = req.bindParameters(EnvInjectJobPropertyInfo.class, "envInjectInfoPrebuildJobProperty."); |
There was a problem hiding this comment.
Method 'StaplerRequest.bindParameters(...)' is deprecated. 
Rename "info" which hides the field declared at line 50.
Split this 136 characters long line (which is greater than 120 authorized).
| } | ||
|
|
||
| private void setContributors(StaplerRequest req, EnvInjectPrebuildJobProperty envInjectJobProperty, JSONObject onJSONObject) { | ||
| if (!onJSONObject.containsKey("contributors")) { |
There was a problem hiding this comment.
Define a constant instead of duplicating this literal "contributors" 3 times.
Creates the "prebuild" phase JobProperty section, allowing scripts to be executed after SCM is completed but before AbstractBuildExecution.doRun kicks in.
Also fixes NPE in EnvInjectJobProperty where disabling a section and saving before applying causes NPE on save