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

Allow retries for pull #994

Closed
wants to merge 3 commits into from
Closed

Allow retries for pull #994

wants to merge 3 commits into from

Conversation

bkelley17
Copy link

Currently the plugin allows configurable retries for push, but not for pull. This change allows retries to be configured for pull as well.

@codecov
Copy link

codecov bot commented Apr 13, 2018

Codecov Report

Merging #994 into master will increase coverage by 0.14%.
The diff coverage is 41.66%.

@@             Coverage Diff              @@
##             master     #994      +/-   ##
============================================
+ Coverage     51.79%   51.93%   +0.14%     
- Complexity     1363     1366       +3     
============================================
  Files           147      147              
  Lines          7457     7456       -1     
  Branches       1132     1132              
============================================
+ Hits           3862     3872      +10     
+ Misses         3229     3217      -12     
- Partials        366      367       +1
Impacted Files Coverage Δ Complexity Δ
...c/main/java/io/fabric8/maven/docker/WatchMojo.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...c/main/java/io/fabric8/maven/docker/BuildMojo.java 0% <0%> (ø) 0 <0> (ø) ⬇️
.../io/fabric8/maven/docker/service/WatchService.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...c/main/java/io/fabric8/maven/docker/StartMojo.java 0% <0%> (ø) 0 <0> (ø) ⬇️
.../io/fabric8/maven/docker/service/BuildService.java 31.25% <0%> (ø) 9 <0> (ø) ⬇️
...ven/docker/access/hc/DockerAccessWithHcClient.java 16.05% <100%> (+2.38%) 14 <0> (+2) ⬆️
.../fabric8/maven/docker/service/RegistryService.java 64% <100%> (ø) 11 <0> (ø) ⬇️
...ocker/access/chunked/BuildJsonResponseHandler.java 13.63% <0%> (+13.63%) 1% <0%> (+1%) ⬆️

@bkelley17
Copy link
Author

I wonder if someone can help me understand what the codecov/patch check is that failed. Is that because coverage of introduced code is not high enough?

@rhuss
Copy link
Collaborator

rhuss commented Apr 16, 2018

@bkelley17 yes, it's because that we generally require that new code should be at least as much tested then the existing code. Not really sure how this is measure as I would expect than if the overall coverage gets increased, then the contribution must be at least have a higher coverage than the rest.

But don't worry too much about it, we also do exceptions from the rule.
Please give me some time to do a review, I hope I can jump on it this week.

thanks for your contribution ...

Copy link
Collaborator

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

LGTM, but let us introduce a pullRetries and pushRetries, too, in addition to the alredady existing retries.

The reason is that we might want to configure this differently for push and pull. So the logic should be:

  • retries given: Use it for both push and pull
  • pushRetries : Use it for push
  • pullRetries : Use it for pull

The properties should be also docker.retries, docker.push.retries and docker.pull.retries.

Could you adapt the PR for this please ? If not, no problem I can do it later, too.

Thanks a lot and sorry for the delay.

@@ -26,6 +26,9 @@
@Parameter(property = "docker.skip.build", defaultValue = "false")
protected boolean skipBuild;

@Parameter(property = "docker.pull.retries", defaultValue = "0")
private int retries;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we please rename this in pullRetries for the member configuration ? "retries" alone is a bit unspecific.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same for the other MoJos please and the documentation.

@rhuss
Copy link
Collaborator

rhuss commented Jan 8, 2020

I will work on this PR for the next release (resolving the conflicts) and add the suggestions as I think this is a valuable feature.

@bkelley17 bkelley17 closed this by deleting the head repository Mar 21, 2023
@rohanKanojia rohanKanojia reopened this Mar 21, 2023
@rohanKanojia rohanKanojia self-assigned this Mar 21, 2023
@rohanKanojia
Copy link
Member

Superseded by #1697

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants