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

upgrade to docker-java-api 3.2.2 #829

Closed

Conversation

Osipion
Copy link

@Osipion Osipion commented Feb 4, 2021

please see #825

notes:

  • relies on docker-java-api being updated to use docker-java 3.2.2 (e.g. DO NOT MERGE :-) )
  • requires docker-java-api remove the maven exclusion on bouncycastle
  • Amazon ECR Credentials plugin no longer works with this.

@pjdarton
Copy link
Member

I think we need to break this up a bit and fix the nasty "the docker-plugin needs to immediately release a new version every time the docker-java-api-plugin changes the docker-java version" issue so we have a wee bit more freedom to upgrade the docker-java-api-plugin without it killing this plugin.
...so I'm working on a PR, jenkinsci/docker-java-api-plugin#9 , which moves the most fragile code in the docker-plugin (fragile because it needs to perfectly match the docker-java code) into the docker-java-api-plugin (where it can be updated each time docker-java is updated).

If/when that's merged, I'll do a "now make use of it" PR to docker-plugin (meaning the the DelegatingDockerClient disappears from this plugin, but then changes to it have to be a part of any "change the version of docker-java" PR on docker-java-api-plugin) and then we merge the current state of play into this PR (which would be a bit smaller as DelegatingDockerClient will've gone from here).

I remain "concerned/unhappy" about the dependencies fallout from this PR, e.g. the Amazon ECR stopping to work sounds like a nasty unwanted side effect; sadly, maven dependencies aren't something I fully understand so it's not something I can help with on a technical front (I'm kinda floundering around in the dark the moment it gets non-trivial, and any "excludes" are non-trivial IMO).

@Osipion
Copy link
Author

Osipion commented Mar 17, 2021

Ok, this seems like a good approach, thanks @pjdarton . As a very temporary measure, I ended up forking Yet Another Docker and patching the changes into that. The benefit YAD has is that it uses the Maven shader plugin on docker-java to directly include docker-java in the project, removing the dependency on docker-java-api. I basically followed a simillar approach to enable ECR authentication - directly copying the relevant code from amazon-ecr-plugin into the project, and refactoring the custom registry credentials YAD uses to make them compatible with the ECR format. In this way I was able to get something that seemed to work fine and minimized the dependencies on the rest of the ecosystem. Definitely not saying that this is the right way to do things though

@pjdarton
Copy link
Member

FYI I have (#836) refactored the pom.xml file so it now uses the Jenkins "BOM" plugin to help with dependency management, bumped the minimum Jenkins version forward, and resolved many of the fixups you needed to address in this PR "all at once".
So, hopefully if you update the pom.xml to be based off the current (as-yet-unreleased) master then you should see the need for a whole load of changes disappear.
Furthermore, if my proposed change to the docker-java-api-plugin go in (which would then result in a new version 3.1.5.3) then that'd allow us to remove the DelegatingDockerClient code from this plugin and thus remove the need to make the changes you needed to do within this PR too.
i.e. that'd make this PR all about changing versions and not having to address old tech-debt at the same time 😉

@pjdarton pjdarton added the dependencies Dependencies label Sep 24, 2021
@pjdarton
Copy link
Member

pjdarton commented Jun 8, 2022

This was resolved by #882 and #884 which allowed this plugin to use later versions of the docker-java-api-plugin.

@pjdarton pjdarton closed this Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants