Skip to content

feat: add support for docker build args to the task run command #5377

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Oct 12, 2023
Merged

feat: add support for docker build args to the task run command #5377

merged 4 commits into from
Oct 12, 2023

Conversation

@briercliffe
Copy link
Contributor

@briercliffe briercliffe commented Oct 11, 2023

Adds the ability to pass build args into the docker build when running the copilot task run command.

Addresses #5376

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

@briercliffe briercliffe requested a review from a team as a code owner October 11, 2023 04:34
@briercliffe briercliffe requested review from CaptainCarpensir and removed request for a team October 11, 2023 04:34
iamhopaul123
iamhopaul123 approved these changes Oct 11, 2023
View reviewed changes
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.

This is so much needed and the implementation is awesome! Thank you very much ❤️

internal/pkg/cli/task_run.go
cmd.Flags().StringVarP(&vars.groupName, taskGroupNameFlag, nameFlagShort, "", taskGroupFlagDescription)

cmd.Flags().StringVar(&vars.dockerfilePath, dockerFileFlag, defaultDockerfilePath, dockerFileFlagDescription)
cmd.Flags().StringToStringVar(&vars.dockerfileBuildArgs, dockerFileBuildArgsFlag, nil, dockerFileBuildArgsFlagDescription)
Copy link
Contributor

@iamhopaul123 iamhopaul123 Oct 11, 2023

Choose a reason for hiding this comment

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

Can you add an example to the [task run help menu] about how we can run task from dockerfile 🙏

Copy link
Contributor Author

@briercliffe briercliffe Oct 12, 2023

Choose a reason for hiding this comment

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

I added an example to the help menu showing how to use the --build-args flag. Thanks for the suggestion!

@github-actions
Copy link

github-actions bot commented Oct 11, 2023 •
edited
Loading

🍕 Here are the new binary sizes!

Name New size (kiB) size (kiB) Delta (%)
macOS (amd) 52052 51976 +0.15
macOS (arm) 52916 52812 +0.20
linux (amd) 45792 45716 +0.17
linux (arm) 45124 45060 +0.14
windows (amd) 43244 43168 +0.18

Lou1415926
Lou1415926 approved these changes Oct 11, 2023
View reviewed changes
Copy link
Contributor

@Lou1415926 Lou1415926 left a comment

Choose a reason for hiding this comment

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

This looks great - thank you! I'm adding the DNM label for you to address iamhopaul123@'s comment!

internal/pkg/cli/flag.go
dockerFileFlagDescription = fmt.Sprintf(`Path to the Dockerfile.
Cannot be specified with --%s.`, imageFlag)
dockerFileBuildArgsFlagDescription = fmt.Sprintf(`Key-value pairs converted to --build-args.
Cannot be specified with --%s.`, imageFlag)
Copy link
Contributor

@Lou1415926 Lou1415926 Oct 11, 2023

Choose a reason for hiding this comment

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

Cannot be specified with --%s.`, imageFlag

aw yay thank you for adding this in the help menu message !

@Lou1415926 Lou1415926 added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Oct 11, 2023
Lou1415926
Lou1415926 approved these changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@Lou1415926 Lou1415926 left a comment

Choose a reason for hiding this comment

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

Thank you!!

@Lou1415926 Lou1415926 removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Oct 12, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2023 •
edited
Loading

Codecov Report

❌ Patch coverage is 44.44444% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.87%. Comparing base (6c673bd) to head (b8eea51).
⚠️ Report is 233 commits behind head on mainline.

Files with missing lines Patch % Lines
internal/pkg/cli/task_run.go 44.44% 5 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff            @@
##           mainline    #5377   +/-   ##
=========================================
  Coverage     69.86%   69.87%           
=========================================
  Files           297      297           
  Lines         45190    45198    +8     
  Branches        287      287           
=========================================
+ Hits          31574    31580    +6     
- Misses        12079    12080    +1     
- Partials       1537     1538    +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mergify mergify bot merged commit 7e7d337 into aws:mainline Oct 12, 2023
KollaAdithya pushed a commit to KollaAdithya/copilot-cli that referenced this pull request Oct 18, 2023
@briercliffe @KollaAdithya
feat: add support for docker build args to the task run command (aws#…
826b683 
…5377)

Adds the ability to pass build args into the docker build when running the `copilot task run` command.


Addresses aws#5376 

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CaptainCarpensir CaptainCarpensir Awaiting requested review from CaptainCarpensir CaptainCarpensir was automatically assigned from aws/aws-ecs-bdx

@efekarakus efekarakus Awaiting requested review from efekarakus

2 more reviewers

@iamhopaul123 iamhopaul123 iamhopaul123 approved these changes

@Lou1415926 Lou1415926 Lou1415926 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

Sprint 🏃‍♀️
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@briercliffe @codecov-commenter @iamhopaul123 @Lou1415926