[JENKINS-48437] Use the new API passing the Run to retrieve the token#141
[JENKINS-48437] Use the new API passing the Run to retrieve the token#141dwnusbaum merged 6 commits intojenkinsci:masterfrom
Conversation
jglick
left a comment
There was a problem hiding this comment.
What about ServerEndpointStep?
jglick
left a comment
There was a problem hiding this comment.
Possibly right. Not sure I understand the nature of the bug or its fix.
| "node {\n" + | ||
| " mockDockerLoginWithEcho {\n" + | ||
| " withDockerRegistry(url: 'https://my-reg:1234', credentialsId: 'registryCreds') {\n" + | ||
| " echo 'config would be set up to connect to https://my-reg:1234'\n" + |
There was a problem hiding this comment.
I am not sure what this is proving. You print a literal message, then later assert that the identical message got printed?
There was a problem hiding this comment.
This echo is not needed for the test. I can remove it. The real test here is that the credentials passed to withDockerRegistry results in the expected docker login command.
| MockAuthorizationStrategy auth = new MockAuthorizationStrategy() | ||
| .grant(Jenkins.READ).everywhere().to("alice") | ||
| .grant(Computer.BUILD).everywhere().to("alice") | ||
| .grant(Item.CONFIGURE).everywhere().to("alice"); |
There was a problem hiding this comment.
I guess for purposes of this test you could just use FullControlOnceLoggedInAuthorizationStrategy or .grant(Jenkins.ADMINISTER).everywhere().toAuthenticated() since the ACL checks are not part of the test, unless I am misunderstanding the nature of the problem.
There was a problem hiding this comment.
The ACL is what JENKINS-48437 is about. When the build runs as a specific user, the credentials cannot be found.
try (ACLContext as = ACL.as(User.getById("alice", false))) {
b = r.buildAndAssertSuccess(p);
}
This tests fails without our fix.
| " }\n" + | ||
| "}", true)); | ||
| WorkflowRun b = story.j.buildAndAssertSuccess(p); | ||
| story.j.assertLogContains("would be connecting to tcp://host:1234", b); |
There was a problem hiding this comment.
But you are not actually testing the credentials here. What then happens with src/main/ reverted? Is there a build failure, or are the credentials ignored, or a warning logged?
There was a problem hiding this comment.
You're right. I am going to add test on DOCKER_TLS_VERIFY and DOCKER_CERT_PATH that should be injected by the step. I have noticed that in the case of JENKINS-48437, there is not failure but those variables are not filled.
|
(Build failure caused by infra "No Space Left on Device") |
JENKINS-48437
Consume a new API introduced in jenkinsci/docker-commons-plugin#68 that accepts a
Runto resolve the credentials ID.