avoid duplicate sending and global notifier mode #66
Conversation
| else | ||
|
|
||
| /* | ||
| * Not pipeline step so @{code build} should be set. |
There was a problem hiding this comment.
style: can we change this to //oneline comment ?
| */ | ||
|
|
||
| /* | ||
| * A pipeline step uses the constructor which sets run. |
There was a problem hiding this comment.
style: can we change this to //oneline comment ?
| private static final long serialVersionUID = 1L; | ||
|
|
||
| @Override | ||
| public OutputStream decorateLogger(Run build, OutputStream logger) throws IOException, InterruptedException | ||
| { | ||
| if (build != null && build instanceof AbstractBuild<?, ?>) | ||
| /* | ||
| * Currently pipeline is not supporting global ConsoleLogFilters. So even when we have it enabled globally |
There was a problem hiding this comment.
I think this would be valid javadoc describing how the method works.
Javadoc would also be more visible in IDEs etc.
| @@ -88,6 +88,11 @@ public void perform(Run<?, ?> run, FilePath workspace, Launcher launcher, | |||
| } | |||
|
|
|||
| private boolean perform(Run<?, ?> run, TaskListener listener) { | |||
| // globally enabled and active. No need to send the data another time. | |||
| if (run.getAction(LogstashMarkerAction.class) != null) | |||
There was a problem hiding this comment.
This effectively makes you unable to have a single-document log generated from any build if global is enabled?
There was a problem hiding this comment.
yes but why you would want single document when you have it already line by line? Currently I don't distinguish between globally and set via JobProperty. We could also add a force option the Notifier.
There was a problem hiding this comment.
IMO best thing would be to have options to enable both modes globally and an ability to override this per-project.
Minimally I'm concerned about a situation when you want to enable the global option but you can't because of some existing projects rely on sending the single document
There was a problem hiding this comment.
Enabling globally to send everything at the end could be done in a RunListener.onFinalized() or onCompleted()with the limitation that it can't fail the build.
onFinalized() would have the advantage that it includes the Final Result line, while onCompleted() would allow to write something to the log but which would not end up in the log sent to the indexer.
There was a problem hiding this comment.
With the RunListener we could even get it right for pipeline as the Result is set when it's run. Avoiding all those questions about how to get the result in.
There was a problem hiding this comment.
Yes, though it is a complex topic and not yet sure how to implement in detail.
E.g. when you enable globally the notifier behaviour (just a single event at the end with the log), how to disable this in a project. Currently I would prefer the JobProperty for this. What if one specifies an explicit notifier in his job while it is also globally enabled. Which one should be taken and what when he wants to fail the build if upload fails (a runlistener can't modify the result anymore)? One could also enable the notifier behaviour via the JobProperty to replace the wrapper behaviour enabled globally. Should we allow a force of the notifier even when globally we have the wrapper enabled.
If we want to have full flexibility we might confuse people when they want to configure the jobs.
Maybe one could reduce it to the following.
A job specific setting overrides a global setting. You can disable the global setting without enabling a job setting. if a job enables the wrapper and configures the notifier only the wrapper will be used.
There was a problem hiding this comment.
Not sure if I understand the "You can disable the global setting without enabling a job setting. " part.
Agree that what is enabled on the job should win with the global setting. Also agree to only choose one if both are configured directly on the job. It would be nice to emit a warning in that case.
There was a problem hiding this comment.
Consider you have enabled it globally but for some reason, you don't want to upload the logs at all for a certain project.
There was a problem hiding this comment.
OK, but how do you propose to handle this? There is no explicit disable option at the moment (you could set maxLines=1)
There was a problem hiding this comment.
via the JobProperty. It gets a checkbox to disable a global setting.
mwinter69
force-pushed
the
noduplicate-sending
branch
from
6f6852f
to
719c87d
Compare
when enabled globally it should be avoided that the notifier or the pipeline steps also send data.
mwinter69
force-pushed
the
noduplicate-sending
branch
from
f898af6
to
547fb62
Compare
add a global notifier mode This allows to upload the tail of the log after each build without a notifier.
add option so the notifier runs at the end and not as a normal notifier. Detect if notifier is wrapper in conditional build step or flexible publisher so it will disable a global configuration
conditional build step plugin is failing the checks
remove unneeded dependency
|
Latest changes now add a global notifier mode via a RunListener. |
mwinter69
changed the title
When enabled globally or via JobProperty it should be avoided that the Notifier also sends data.
Pipeline step should also avoid it, but this depends on [JENKINS-45693]. Once this bug is fixed it should also work for pipeline.