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

[JENKINS-40700] Enable Launcher to hold the communication protocol. #452

Merged
merged 1 commit into from
May 13, 2021

Conversation

Aki-7
Copy link
Member

@Aki-7 Aki-7 commented May 8, 2021

I'm working on the ticket below.
[JENKINS-40700] Display communication protocol in agent logs

And this is the continuation of #450.
#450 enables the current Engine to hold the name of the using communication protocol such as JNLP4-connect.

This PR enables Launcher to hold communication protocol name such as TCP (remote: client) or Standard in/out

I submitted the PR that uses this feature on the controller side to jenkinsci / jenkins.
link: jenkinsci/jenkins#5459

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your master branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@oleg-nenashev oleg-nenashev added the enhancement For changelog: An enhancement providing new capability. label May 8, 2021
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.

Thanks! It would be nice to add it. At the same time, Javadoc for this and for #450 methods are needed IMHO

@@ -787,6 +790,10 @@ public static boolean isWindows() {
return File.pathSeparatorChar==';';
}

public static String getCommunicationProtocol() {
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add Javadoc to this method and to getProtocolName()? They are very close, and disambiguation is needed. Also, it could be a "getCommunicationProtocolName"

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand. Thank you for reviewing!

@Aki-7
Copy link
Member Author

Aki-7 commented May 8, 2021

[Resolved]

The CI process for Windows has been canceled but the error log seems unrelated to my changes. Do you have any ideas? Or if it is a temporary error due to networking or something, can I just rerun the pipeline? (I'm not privileged to rerun it).
I pushed the same commit again to rerun the pipeline.

Log message:


Cannot contact EC2 (aws) - Windows 2019 (i-09a1c08cae6665e79): hudson.remoting.ChannelClosedException: Channel "hudson.remoting.Channel@40d5902d:EC2 (aws) - Windows 2019 (i-09a1c08cae6665e79)": Remote call on EC2 (aws) - Windows 2019 (i-09a1c08cae6665e79) failed. The channel is closing down or has closed down


link: https://ci.jenkins.io/blue/organizations/jenkins/Core%2Fremoting/detail/PR-452/2/pipeline/

Save the communication protocol when the channel is established and
provide the protocol name via `getCommunicationProtocolName`.
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.

Looks good to me! As a best practice, it would be nice to also document nullability of the API by adding @CheckForNull or @NonNull annotations

@Aki-7
Copy link
Member Author

Aki-7 commented May 9, 2021

I got it. Thank you for your help!

@oleg-nenashev oleg-nenashev merged commit 18bebb2 into jenkinsci:master May 13, 2021
@oleg-nenashev
Copy link
Member

Thanks for the patch @Aki-7 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement For changelog: An enhancement providing new capability. ready-to-merge
Projects
None yet
3 participants