-
Notifications
You must be signed in to change notification settings - Fork 141
OutboundAgent test utility
#607
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
Conversation
| rule.jenkins.addNode(new DumbSlave( | ||
| name, | ||
| "/home/jenkins/agent", | ||
| new SSHLauncher(connectionDetails.host, connectionDetails.port, creds.getId()))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No compelling reason to integrate with #570 for now, since the utility deliberately papers over details about how the agent is set up: it just needs to live “somewhere else”.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to also enable authentication via username and password?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be a different test, as one already exists. The point of this test is to use the official outboundAgent Docker image. We use an Ubuntu Docker image on purpose in the other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to also enable authentication via username and password?
No, because we do not care how the launcher works, just that it does. This is a test utility, not a test of the SSH launcher.
| package test.ssh_agent; | ||
|
|
||
| import hudson.model.Slave; | ||
| import org.junit.Rule; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Could it be JUnit 5? we recently migrate testcontainers test to JUnit 5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the test uses RealJenkinsRule, this would require jenkinsci/jenkins-test-harness#988 or similar in order to work with JUnit 5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like this one #562
No description provided.