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

chore: default the writer of docker build to Stderr incase of single image #5148

Merged
merged 4 commits into from
Aug 8, 2023

Conversation

KollaAdithya
Copy link
Contributor

@KollaAdithya KollaAdithya commented Aug 1, 2023

related #5124, #5067
This PR will remove the rolling display when there is only one image to build.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

@KollaAdithya KollaAdithya requested a review from a team as a code owner August 1, 2023 01:51
@KollaAdithya KollaAdithya requested review from huanjani and removed request for a team August 1, 2023 01:51
@github-actions
Copy link

github-actions bot commented Aug 1, 2023

🍕 Here are the new binary sizes!

Name New size (kiB) size (kiB) Delta (%)
macOS (amd) 51500 51488 +0.02
macOS (arm) 51716 51680 +0.07
linux (amd) 45332 45324 +0.02
linux (arm) 43584 43588 ❤️ -0.01
windows (amd) 42152 42144 +0.02

@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2023

Codecov Report

Merging #5148 (b35129f) into mainline (83671d8) will decrease coverage by 0.04%.
Report is 2 commits behind head on mainline.
The diff coverage is 95.00%.

@@             Coverage Diff              @@
##           mainline    #5148      +/-   ##
============================================
- Coverage     69.51%   69.48%   -0.04%     
============================================
  Files           295      295              
  Lines         43333    43387      +54     
  Branches        285      285              
============================================
+ Hits          30124    30148      +24     
- Misses        11740    11767      +27     
- Partials       1469     1472       +3     
Files Changed Coverage Δ
internal/pkg/cli/deploy/workload.go 60.18% <95.00%> (+0.47%) ⬆️

... and 7 files with indirect coverage changes

@dannyrandall dannyrandall added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Aug 2, 2023
@KollaAdithya KollaAdithya removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Aug 2, 2023
@huanjani
Copy link
Contributor

huanjani commented Aug 3, 2023

Could we add a unit test for multiple images?

@KollaAdithya
Copy link
Contributor Author

KollaAdithya commented Aug 3, 2023

I think we already had a test case for this scenario of multiple images over here
https://github.com/aws/copilot-cli/pull/5148/files#diff-cc7a53c33c7639692deae85af8e5ec1187f3ef5dc845b29f53f9ae698043ef7cR311

Copy link
Contributor

@huanjani huanjani left a comment

Choose a reason for hiding this comment

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

lgtm!

@@ -401,6 +401,7 @@ func (d *workloadDeployer) uploadContainerImages(out *UploadArtifactsOutput) err
if len(buildArgsPerContainer) == 0 {
return nil
}
isMultipleContainerImages := len(buildArgsPerContainer) > 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make more sense for us to call a separate function if isMultipleContainerImages is true (after repository.Login())? The reason is we probably don't need the parallel part at all for single build case and it'll be easier to read/maintain

Copy link
Contributor

@iamhopaul123 iamhopaul123 left a comment

Choose a reason for hiding this comment

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

awesome :shipit:

@KollaAdithya KollaAdithya added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Aug 8, 2023
@KollaAdithya KollaAdithya removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Aug 8, 2023
@mergify mergify bot merged commit d0ec26c into aws:mainline Aug 8, 2023
12 checks passed
Sprint 🏃‍♀️ automation moved this from In review to Pending release Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Sprint 🏃‍♀️
  
Pending release
Development

Successfully merging this pull request may close these issues.

None yet

5 participants