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

Don't create an ephemeral builder if it isn't truly needed #2196

Merged
merged 3 commits into from
Jul 8, 2024

Conversation

natalieparellano
Copy link
Member

Fixes #2195

Summary

Output

Before

After

Documentation

  • Should this change be documented?
    • Yes, see #___
    • No

Related

Resolves #___

@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Jul 2, 2024
@github-actions github-actions bot added this to the 0.35.0 milestone Jul 2, 2024
Copy link

codecov bot commented Jul 2, 2024

Codecov Report

Attention: Patch coverage is 68.75000% with 15 lines in your changes missing coverage. Please review.

Project coverage is 70.16%. Comparing base (2fa3260) to head (376513b).

Additional details and impacted files

Impacted file tree graph

@@            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     
Flag Coverage Δ
os_linux 69.30% <68.75%> (+0.03%) ⬆️
os_macos-arm64 65.62% <68.75%> (+0.02%) ⬆️
os_windows 69.68% <68.75%> (-0.01%) ⬇️
unit 70.16% <68.75%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@natalieparellano natalieparellano marked this pull request as ready for review July 3, 2024 13:28
@natalieparellano natalieparellano requested review from a team as code owners July 3, 2024 13:28
@jjbustamante
Copy link
Member

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

Copy link
Member

@jjbustamante jjbustamante left a 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

@natalieparellano
Copy link
Member Author

Do we have an acceptance test covering this scenario?

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.

@jjbustamante
Copy link
Member

Do we have an acceptance test covering this scenario?

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

@natalieparellano natalieparellano merged commit 98c98da into main Jul 8, 2024
18 checks passed
@natalieparellano natalieparellano deleted the lazy-ephemeral-builder branch July 8, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pack should not create an ephemeral builder if it isn't truly needed
2 participants