-
Notifications
You must be signed in to change notification settings - Fork 293
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
Don't create an ephemeral builder if it isn't truly needed #2196
Conversation
Fixes #2195 Signed-off-by: Natalie Arellano <narellano@vmware.com>
Signed-off-by: Natalie Arellano <narellano@vmware.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2196 +/- ##
==========================================
- Coverage 70.17% 70.16% -0.01%
==========================================
Files 253 253
Lines 18452 18495 +43
==========================================
+ Hits 12947 12975 +28
- Misses 4659 4671 +12
- Partials 846 849 +3
Flags with carried forward coverage won't be shown. Click here to find out more. |
Do we have an acceptance test covering this scenario? The only thing that comes to my mind is we are not trying to save the ephemeral builder at some point during the build and we end up introducing some bug |
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.
LGTM
My only concern is to double-check the acceptance test coverage
I didn't add an explicit test for this, because I am lazy (and also because our current suite is long enough) but the tests were failing before I added 423f596 because we were cleaning up the original builder, making it unavailable for other tests. |
Then I am ok with it |
Fixes #2195
Summary
Output
Before
After
Documentation
Related
Resolves #___