Added aditional params option #98
Conversation
ghost
changed the title
jakub-bochenski
left a comment
jakub-bochenski
left a comment
There was a problem hiding this comment.
Thanks for the PR, can you please remove the unnecessary formatting changes to make review easier?
|
Hey mate, fixed it. Also added a UT. Very appreciated, waiting for your CR
בתאריך יום ב׳, 8 בפבר׳ 2021 ב-17:27 מאת jakub-bochenski <
notifications@github.com>:
… ***@***.**** requested changes on this pull request.
Thanks for the PR, can you please remove the unnecessary formatting
changes to make review easier?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#98 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKBTYUAWUOXM3EWW3BFR4MLS577F7ANCNFSM4XHKQCHQ>
.
|
ghost
changed the title
Yes I fixed it |
ghost
requested a review
from 
|
sorry for the delay, I will try to review this by EOW |
src/main/java/jenkins/plugins/logstash/persistence/BuildData.java
Outdated
Show resolved
Hide resolved
| this(build, currentTime, listener, null); | ||
| } | ||
| // Freestyle project build | ||
| public BuildData(AbstractBuild<?, ?> build, Date currentTime, TaskListener listener, Map<String, String> additionalParams) { |
There was a problem hiding this comment.
it seems there is no way to use this additional params in freestile builds, right?
There was a problem hiding this comment.
I don't see any stapler bindings updated, how would it be configured?
There was a problem hiding this comment.
For now it won't be implemented in freestyle builds
There was a problem hiding this comment.
OK, can you please remove those extra constructors?
Otherwise LGTM
jakub-bochenski
left a comment
jakub-bochenski
left a comment
There was a problem hiding this comment.
Can you please add documentation/example to readme on how to use thix
src/main/java/jenkins/plugins/logstash/persistence/BuildData.java
Outdated
Show resolved
Hide resolved
src/main/java/jenkins/plugins/logstash/pipeline/LogstashSendStep.java
Outdated
Show resolved
Hide resolved
|
@zivOrLive do you intend to finish work on this. Do you need help? |
Im not sure how to adjust it so you could use it in freestyle builds, if you could direct me it would be of much help (Im pretty new to jenkins) |
|
It's OK, it can be a Pipeline only feature at first. |
src/main/java/jenkins/plugins/logstash/persistence/BuildData.java
Outdated
Show resolved
Hide resolved
jakub-bochenski
left a comment
jakub-bochenski
left a comment
There was a problem hiding this comment.
Please apply those 2 more corrections, otherwise LGTM
|
@mwinter69 please take a look if you have time/feel like it :) |
1 participant
I have noticed the lack of ability to send additional environment variables, and so I added an ability to pass a HashMap<String,String> of additional params