Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Console logfilter and workflow #1833

Closed
wants to merge 2 commits into from

Conversation

8 participants
@kohsuke
Copy link
Member

commented Sep 18, 2015

Generalized ConsoleLogFilter to work with Run and not just AbstractBuild.

@kohsuke

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2015

@reviewbybees

This comment has been minimized.

Copy link

commented Sep 18, 2015

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

*/
public abstract OutputStream decorateLogger(AbstractBuild build, OutputStream logger) throws IOException, InterruptedException;
public OutputStream decorateLogger(AbstractBuild build, OutputStream logger) throws IOException, InterruptedException {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Sep 18, 2015

Member

Also needs @Deprecated annotation

* Even though this method is not marked 'abstract', this is the method that must be overridden
* by extensions.
*/
public OutputStream decorateLogger(Run build, OutputStream logger) throws IOException, InterruptedException {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Sep 18, 2015

Member

Do we want to introduce raw classes in interfaces? Probably Run is better


if (build instanceof AbstractBuild) {
// maybe the plugin implements the old signature.
return decorateLogger((AbstractBuild) build, logger);

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Sep 18, 2015

Member

Is there is a risk of an infinite call loop between methods?

This comment has been minimized.

Copy link
@amuniz

amuniz Sep 18, 2015

Member

The only possibility would be to have an old implementation calling super.decorateLogger(AbstractBuild, OutputStream), but it's not possible in this case since the original signature was abstract.

This comment has been minimized.

Copy link
@jglick

jglick Sep 18, 2015

Member

Would love to have a test utility which allows you to actually prove arguments like this. I for one always get confused by it, and have made mistakes.

* the data that is written to the log.
*
* <p>
* Even though this method is not marked 'abstract', this is the method that must be overridden

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Sep 18, 2015

Member

Would it be possible to somehow notify developers when they update the core dependency? Probably there is an annotation for it (in accmod?)

This comment has been minimized.

Copy link
@jglick

jglick Sep 18, 2015

Member

There is not. It would be very useful in this and similar cases to have an @Abstract annotation with an associated processor which rejects class that do not override the method. @kohsuke POTD?

@amuniz

This comment has been minimized.

Copy link
Member

commented Sep 18, 2015

🐝

@KostyaSha

This comment has been minimized.

Copy link
Member

commented Sep 18, 2015

Reference implementation?

@jtnord

This comment has been minimized.

Copy link
Member

commented Sep 18, 2015

what no jenkins JIRA for the changelog.?

@stephenc

This comment has been minimized.

Copy link
Member

commented Sep 18, 2015

🐝 on the technicals... 🐜 for not having an associated JENKINS-___ issue number to track

@jglick

This comment has been minimized.

Copy link
Member

commented Sep 18, 2015

🐛 for lack of any demonstration that this works, or explanation of what user problem is being solved. Would expect to see (at least) two downstream PRs:

  • from a plugin implementing ConsoleLogFilter; jenkins.python.expoint.ConsoleLogFilterPW is the candidate that I can find
  • from workflow-plugin, calling the new overload (it does not use RunExecution, so that part of the patch is useless)

Normally for Workflow ConsoleLogFilter is only called via SimpleBuildWrapper.createLoggerDecorator, in which case the build parameter is simply null. The only unhandled use case is a globally-registered ConsoleLogFilter.

@kohsuke

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2015

I've filed JENKINS-30777 for this, ammended commit messages, and used the validated merge to send the change in for 1.632.

@kohsuke kohsuke closed this Oct 3, 2015

@jglick jglick deleted the console-logfilter-and-workflow branch Oct 5, 2015

@jglick

This comment has been minimized.

Copy link
Member

commented Oct 5, 2015

Actual patch here. This added a new overload decorateLogger(Computer computer, OutputStream) for the slave log, unrelated to the build log; still no evidence that the original change accomplished anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.