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

Fix fabric8:push goal in auto mode #1806

Merged
merged 1 commit into from
Mar 31, 2020

Conversation

rohanKanojia
Copy link
Member

fabric8:push goal seems to be failing due to no docker deamon
initialization being done. The cause seemed to be function isDockerAccessRequired()
which was ignoring the fact that docker access is also used in openshift mode. If we
don't specify any mode explicitly it is set to RuntimeMode.auto.

I think we should disable docker access check only when JIB=true

@rohanKanojia
Copy link
Member Author

@toschder : I've created this PR for your issue. Could you please check if it's working for you?

@@ -157,7 +166,7 @@ public void executeInternal(ServiceHub hub) throws DockerAccessException, MojoEx
jibPush(imageConfiguration, project, getRegistryConfig(pushRegistry), outputDirectory, log);
}
} else {
super.executeInternal(hub);
hub.getRegistryService().pushImages(getResolvedImages(), retries, getRegistryConfig(pushRegistry), skipTag);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the PR description, this has nothing to do with the issue.

Why can't we leave the call to super.excuteInternal(hub)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I did this to resolve docker.push.registry, super.executeInternal(hub) delegates to dmp's PushMojo where pushRegistry is null, we can make it protected in dmp but we would be blocked on updating dmp to new version.

If you want I can revert to previous state too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see the problem now.

The value for the property docker.push.registry aliased as pushRegistry is required in JIB and variable must be "overridden">overwritten 🤢

There's an alternative to this (which is not ideal either) but that can be used to at least reuse some of the code (It's pointless to extend io.fabric8.maven.docker.PushMojo when we're not reusing anything at all).

The alternative is to define an extra variable with a different name and set the @Parameter annotation to pick up the values accordingly.
e.g.

    @Parameter(alias = "pushRegistry", property = "docker.push.registry")
    private String fabric8PushRegistry;

This way, the value of the parameter will be available through the whole class hierarchy (in this case FMP's PushMojo and DMP's PushMojo) without the need to redefine access privileges in parent classes. (Check marcnuri-forks@86d60b8 to see the changes)

For the current case, given that we're not using anything from the parent PushMojo (DMP) I would change the class to extend from AbstractDockerMojo.

But it's really important that you keep in mind the above procedure when you require an inaccessible parameter from a parent class, as overwriting them will lead to bugs.

manusa added a commit to marcnuri-forks/fabric8-maven-plugin that referenced this pull request Mar 28, 2020
fabric8:push goal seems to be failing due to no docker deamon
initialization being done. The cause seemed to be function isDockerAccessRequired()
which was ignoring the fact that docker access is also used in openshift mode. If we
don't specify any mode explicitly it is set to RuntimeMode.auto.

I think we should disable docker access check only when JIB=true
@manusa manusa merged commit 23f7c1d into fabric8io:master Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants