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

Added support for 'cachefrom' when building an image #1139

Merged
merged 6 commits into from
Apr 21, 2019

Conversation

llowrey
Copy link
Contributor

@llowrey llowrey commented Dec 12, 2018

Fixes #1132 :This is my initial attempt. I'm not sure how to test it so this effort is incomplete. This PR relates to issue

@codecov
Copy link

codecov bot commented Dec 12, 2018

Codecov Report

Merging #1139 into master will decrease coverage by 0.04%.
The diff coverage is 26.66%.

@@             Coverage Diff              @@
##             master    #1139      +/-   ##
============================================
- Coverage     52.29%   52.25%   -0.05%     
- Complexity     1472     1474       +2     
============================================
  Files           150      150              
  Lines          7852     7866      +14     
  Branches       1168     1171       +3     
============================================
+ Hits           4106     4110       +4     
- Misses         3347     3355       +8     
- Partials        399      401       +2
Impacted Files Coverage Δ Complexity Δ
...8/maven/docker/config/BuildImageConfiguration.java 78.48% <10%> (-3.73%) 61 <1> (+1)
.../io/fabric8/maven/docker/service/BuildService.java 32.03% <100%> (+0.53%) 9 <0> (ø) ⬇️
...a/io/fabric8/maven/docker/access/BuildOptions.java 91.3% <50%> (-8.7%) 15 <1> (+1)

1 similar comment
@codecov
Copy link

codecov bot commented Dec 12, 2018

Codecov Report

Merging #1139 into master will decrease coverage by 0.04%.
The diff coverage is 26.66%.

@@             Coverage Diff              @@
##             master    #1139      +/-   ##
============================================
- Coverage     52.29%   52.25%   -0.05%     
- Complexity     1472     1474       +2     
============================================
  Files           150      150              
  Lines          7852     7866      +14     
  Branches       1168     1171       +3     
============================================
+ Hits           4106     4110       +4     
- Misses         3347     3355       +8     
- Partials        399      401       +2
Impacted Files Coverage Δ Complexity Δ
...8/maven/docker/config/BuildImageConfiguration.java 78.48% <10%> (-3.73%) 61 <1> (+1)
.../io/fabric8/maven/docker/service/BuildService.java 32.03% <100%> (+0.53%) 9 <0> (ø) ⬇️
...a/io/fabric8/maven/docker/access/BuildOptions.java 91.3% <50%> (-8.7%) 15 <1> (+1)

@rhuss
Copy link
Collaborator

rhuss commented Dec 13, 2018

Thanks for the PR ! To complete it, I'd recommend:

  • Add a test method to BuildConfigTest to ensure that the generated JSON is as you expect
  • Add to the build options in the docs
  • Add an entry in doc/changelog.md

We don't have really any integration tests, so its ok to have only this unit test.

I'm planning to make a release this evening (CET), so we could still include this PR if you have time to implement that parts today.

thanks !

@rohanKanojia
Copy link
Member

@llowrey : Is this till a WIP?

@rohanKanojia rohanKanojia requested a review from rhuss March 2, 2019 10:26
@llowrey
Copy link
Contributor Author

llowrey commented Mar 2, 2019

This should be done. I had made the changes requested by @rhuss.

@rohanKanojia
Copy link
Member

@llowrey: ah, okay. Adding WIP label then

@rohanKanojia rohanKanojia added the WIP Work in Progress label Mar 2, 2019
@rohanKanojia
Copy link
Member

@llowrey : Polite ping, still working on this patch? We're preparing to release and I would like to have this in. Could you please find time to update this? Otherwise we would update it whenever we get time.

@llowrey
Copy link
Contributor Author

llowrey commented Apr 5, 2019

This should have been done as of my commit on Dec-14. Is there something else I need to do?

@rohanKanojia
Copy link
Member

@llowrey : oh, sorry. I misunderstood your previous statement. Removing WIP then

@rohanKanojia rohanKanojia removed the WIP Work in Progress label Apr 5, 2019
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.

Thank for the PR ! Could you please add the new property also to PropertyHandler and PropertyHandlerTest ? I.e. for usage als properties. Also, the documentation needs to be updated here.

@llowrey
Copy link
Contributor Author

llowrey commented Apr 7, 2019

Unfortunately, I do not have any more time to put toward this so I am abandoning this PR. Sorry for the bother and thank you for considering it.

@llowrey llowrey closed this Apr 7, 2019
@rohanKanojia
Copy link
Member

@llowrey : ah, It's okay someone else can pick this up. It's pretty much done, right? What are things that need to be fixed in order to get this merged?

@rhuss
Copy link
Collaborator

rhuss commented Apr 7, 2019

@rohanKanojia actually I think its only the integration into the PropertyHandler (including a test). Otherwise the PR looks good to me.

@rhuss rhuss reopened this Apr 7, 2019
@llowrey
Copy link
Contributor Author

llowrey commented Apr 7, 2019

@rohanKanojia: I don't know. @rhuss mentioned PropertyHandler and PropertyHandlerTest but I couldn't figure out what to do after poking around for a few minutes. I'm just not familiar enough with the whole codebase to know what else needs to be done. The feature works, at least in my use case. It seems what remains is to do whatever else is required for it to be properly integrated into the codebase.

@rhuss
Copy link
Collaborator

rhuss commented Apr 7, 2019

@llowrey no worries we can pick that up, if this is ok for you.

@rhuss
Copy link
Collaborator

rhuss commented Apr 7, 2019

@llowrey thanks for the PR btw, really appreciated !

@rhuss rhuss added this to the 0.30.0 milestone Apr 21, 2019
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.

Thanks ! lgtm.

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.

Add support for cachefrom
3 participants