Skip to content
New issue

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

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

Already on GitHub? # to your account

[JENKINS-27035] Gathering command read/write events #3071

Merged
merged 20 commits into from
Feb 6, 2018

Conversation

jglick
Copy link
Member

@jglick jglick commented Oct 10, 2017

JENKINS-27035

Downstream of jenkinsci/remoting#204.

@reviewbybees

@jglick jglick added the work-in-progress The PR is under active development, not ready to the final review label Oct 10, 2017
pom.xml Outdated
@@ -170,7 +170,7 @@ THE SOFTWARE.
<dependency>
<groupId>org.jenkins-ci.main</groupId>
<artifactId>remoting</artifactId>
<version>3.15-20171128.150341-4</version> <!-- TODO https://github.com/jenkinsci/remoting/pull/204 -->
<version>3.15-SNAPSHOT</version> <!-- TODO https://github.com/jenkinsci/remoting/pull/204 -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See cab2663 commit message.

@jglick jglick removed the work-in-progress The PR is under active development, not ready to the final review label Dec 18, 2017
@oleg-nenashev oleg-nenashev self-assigned this Dec 22, 2017
@oleg-nenashev oleg-nenashev added the on-hold This pull request depends on another event/release, and it cannot be merged right now label Dec 22, 2017
@oleg-nenashev
Copy link
Member

on hold till the new Remoting version gets released

// following Ant <mkdir> task to avoid possible race condition.
Thread.sleep(10);
// following Ant <mkdir> task to avoid possible race condition.
Thread.sleep(10);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh oh, do we really have THAT in FilePath?

@oleg-nenashev
Copy link
Member

I feel to could be moved to an API class in Remoting. E.g.

class LoggerChannelListener extends ChannelListener {
    private final Logger LOGGER;

    public LoggingChannelListener() {
       ...
    }    
 
    ... // all methods
}

Why? It may be useful to have the same logger for Remoting CLI, Maven Plugin and other custom implementations with Channel.

WDYT @jglick ?

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐝 in general, but I feel the Logging listener should be an API class in Remoting itself

@jglick
Copy link
Member Author

jglick commented Jan 2, 2018

Pushing down the logging listener is a reasonable idea, though it is a little tricky since then core would either need to attach two listeners (slightly more runtime overhead) or subclass it (somewhat poor style).

@daniel-beck daniel-beck added the unresolved-merge-conflict There is a merge conflict with the target branch. label Jan 8, 2018
@ghost
Copy link

ghost commented Jan 11, 2018

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.

@jglick jglick removed on-hold This pull request depends on another event/release, and it cannot be merged right now unresolved-merge-conflict There is a merge conflict with the target branch. labels Jan 19, 2018
@oleg-nenashev
Copy link
Member

I hope to release the Remoting side next week

@jglick
Copy link
Member Author

jglick commented Jan 31, 2018

Needs a few test fixes to keep up with jenkinsci/remoting#247.

@jglick jglick added the needs-more-reviews Complex change, which would benefit from more eyes label Jan 31, 2018
@oleg-nenashev
Copy link
Member

oleg-nenashev commented Jan 31, 2018

It will need an integration with the master branch once #3276 is released

@oleg-nenashev
Copy link
Member

Crap, I forgot to merge it. #FOSDEM

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
needs-more-reviews Complex change, which would benefit from more eyes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants