don’t lookup systemCredentials to eventually store credentialsId which was initial constructor parameter #82
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…h was initial constructor parameter Running Configuration-as-Code we inject credentialsId during init, while the credentials store might not be setup yet so it can resolve it to an actual credentials instance. Signed-off-by: Nicolas De Loof <[email protected]>
|
issue with DockerRule "Failed to build image (100): ff2527313f5c" |
Reading package lists...
Reading package lists...
Building dependency tree...
Some packages could not be installed. This may mean that you have
requested an impossible situation or if you are using the unstable
distribution that some required packages have not yet been created
or been moved out of Incoming.
The following information may help to resolve the situation:
The following packages have unmet dependencies:
openssh-server : Depends: openssh-client (= 1:7.5p1-10) but 1:7.5p1-10ubuntu0.1 is to be installed
Recommends: libpam-systemd but it is not going to be installed
Recommends: ncurses-term but it is not going to be installed
Recommends: xauth but it is not going to be installed
Recommends: ssh-import-id but it is not going to be installed
E: Unable to correct problems, you have held broken packages.
The command '/bin/sh -c apt-get update -y && apt-get install -y openssh-server=1:7.5p1-10' returned a non-zero code: 100 |
Signed-off-by: Nicolas De Loof <[email protected]>
2 tasks
oleg-nenashev
requested changes
Feb 21, 2018
| @@ -853,7 +863,7 @@ public Boolean call() throws InterruptedException { | |||
| final Node node = computer.getNode(); | |||
| final String nodeName = node != null ? node.getNodeName() : "unknown"; | |||
| if(node != null) { | |||
| CredentialsProvider.track(node, credentials); | |||
| CredentialsProvider.track(node, getCredentials()); | |||
There was a problem hiding this comment.
Calling a non-final getter in the constructor is generally a bad idea. SSHLauncher is extended in several plugins, and this method is overridden: https://github.com/jenkinsci/docker-plugin/blob/5a2a123cd808a9c82d154cbcc22ef906a6a97907/src/main/java/io/jenkins/docker/connector/DockerComputerSSHConnector.java#L417-L434
There was a problem hiding this comment.
this is not constructor but launch method body.
Maybe you've been confused by github UI to hardly capture the context with a Callable involved
|
@reviewbybees tho |
oleg-nenashev
approved these changes
Feb 23, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SSHLauncher do receive
credentialsIdbut then retrieve matchingCredentialsinstance, then storescredentials.getI(). Proposed change make it just simpler by storing IDinitial reason doing so :
Running Configuration-as-Code we inject credentialsId during init, while the credentials store might not be setup yet so it can resolve it to an actual credentials instance.
Signed-off-by: Nicolas De Loof [email protected]