-
Notifications
You must be signed in to change notification settings - Fork 98
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
Remove the deprecated things and replace Fabric8 Maven plugin with JKube maven plugin #1309
Conversation
c575035
to
70bbc35
Compare
… tests in containerless module on CircleCI
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.
Hi @jimma - I had a quick look, and have just a minor comment, since I didn't dive in properly yet for a detailed code review.
In the meanwhile, I am asking whether we can deal with #1311 separately and I'd ask the same for the Fabric8 -> JKube Maven plugin (where maybe I could help, as well).
WDYT?
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.
Thanks again for your work on these changes, @jimma
I've gone through the commits you added, and the incremental changes the introduce.
Regarding the boot2Docker and Docker Machine modules removal, I am fine with the changes and dropped just one question about a commented out block.
Again, regarding Fabric8 Maven plugin -> JKube Maven plugin, I am not sure I get the reasoning for not addressing it in a separate PR, but maybe I am missing something.
Could you please elaborate on this? I know we discussed the replacement in chat, but a dedicated issue/PR would be better IMHO.
Update: now I see that the related issue (#1308) is actually more generic, therefore the Fabric8->Jkube related changes could fit in here.
If that is the case @jimma - would you please change the PR title to either mention both boot2Docker and Fabric8 Maven plugin removal/replacement or none of them?
...er/src/main/java/org/arquillian/cube/docker/impl/client/CubeDockerConfigurationResolver.java
Outdated
Show resolved
Hide resolved
...java/org/eclipse/jkube/maven/plugin/build/KubernetesMavenPluginResourceGeneratorBuilder.java
Outdated
Show resolved
Hide resolved
Thanks @fabiobrz for the review again. |
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.
@jimma one minor comment on the latest tag usage in the DockerTemplate, others LGTM :)
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.
Thanks @jimma - changes LGTM. Approving.
Short description of what this resolves:
Remove deprecated things
Changes proposed in this pull request:
Fixes #1308