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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-59777] Allow nested docker.withRegistry() blocks #82

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[JENKINS-59777] Allow nested docker.withRegistry() blocks #82

wants to merge 2 commits into from

Conversation

arthurvanduynhoven
Copy link

@arthurvanduynhoven arthurvanduynhoven mentioned this pull request Oct 15, 2019
Closed
@arthurvanduynhoven
Copy link
Author

@oleg-nenashev Can you review?

@oleg-nenashev oleg-nenashev self-requested a review December 10, 2019 21:40
@oleg-nenashev
Copy link
Member

I do not really maintain it since 2017, and my bandwidth is not enough to review all PRs timely. But I added it to my queue. Also CC @jenkinsci/code-reviewers

@arthurvanduynhoven
Copy link
Author

thanks for adding it to your list of TODOs :)

oleg-nenashev
oleg-nenashev requested changes Dec 23, 2019
View reviewed changes
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 for the pull request @arthurvanduynhoven !
Right now there is no active maintainers, but I might be available to cut a new release once the code is ready to go. At the moment it is missing autotests which reproduce the issue and demonstrate the fix. It would be great to have some. Also, you could merge the code with the master branch and document the use-case in README. The rest of the comments are either codestyle or performance overhead ones

src/main/java/org/jenkinsci/plugins/docker/commons/impl/RegistryKeyMaterialFactory.java Outdated Show resolved Hide resolved
src/main/java/org/jenkinsci/plugins/docker/commons/impl/RegistryKeyMaterialFactory.java Outdated Show resolved Hide resolved
src/main/java/org/jenkinsci/plugins/docker/commons/impl/RegistryKeyMaterialFactory.java Outdated Show resolved Hide resolved
src/main/java/org/jenkinsci/plugins/docker/commons/impl/RegistryKeyMaterialFactory.java Outdated Show resolved Hide resolved
src/main/java/org/jenkinsci/plugins/docker/commons/impl/RegistryKeyMaterialFactory.java Outdated Show resolved Hide resolved
@arthurvanduynhoven arthurvanduynhoven force-pushed the JENKINS-59777-AllowNestedDockerCredentialBlocks branch from 5ba1316 to 4f52fae Compare January 9, 2020 13:06
@oleg-nenashev oleg-nenashev self-assigned this Jan 17, 2020
@arthurvanduynhoven
Copy link
Author

@oleg-nenashev Did the updates sufficient for merge now?

@arthurvanduynhoven arthurvanduynhoven force-pushed the JENKINS-59777-AllowNestedDockerCredentialBlocks branch from 4f52fae to 8be89fc Compare July 6, 2020 07:01
@mswezey23
Copy link

Any update on this? Would love me some nested logins

@arthurvanduynhoven arthurvanduynhoven force-pushed the JENKINS-59777-AllowNestedDockerCredentialBlocks branch from 8be89fc to c87ea63 Compare March 10, 2022 07:51
@arthurvanduynhoven
Copy link
Author

@rsandell Since you seem to be doing some changes - possible for you to review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleg-nenashev oleg-nenashev Awaiting requested review from oleg-nenashev

Assignees

@oleg-nenashev oleg-nenashev

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@arthurvanduynhoven @oleg-nenashev @mswezey23