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

Add option to skaffold run similar to "skaffold build -b" #4734

Merged
merged 4 commits into from
Sep 21, 2020

Conversation

anshlykov
Copy link
Contributor

Fixes: #4494

Description
Added the artifact filter for the run command, similar to the build command.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@codecov
Copy link

codecov bot commented Aug 28, 2020

Codecov Report

Merging #4734 into master will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4734      +/-   ##
==========================================
+ Coverage   71.82%   71.91%   +0.08%     
==========================================
  Files         347      347              
  Lines       11869    11892      +23     
==========================================
+ Hits         8525     8552      +27     
+ Misses       2725     2718       -7     
- Partials      619      622       +3     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/build.go 89.18% <ø> (+4.97%) ⬆️
cmd/skaffold/app/cmd/flags.go 82.85% <ø> (ø)
cmd/skaffold/app/cmd/run.go 87.50% <100.00%> (+37.50%) ⬆️
pkg/skaffold/build/tag/util.go 89.47% <0.00%> (-10.53%) ⬇️
cmd/skaffold/app/cmd/deploy.go 68.75% <0.00%> (+2.08%) ⬆️
cmd/skaffold/app/tips/tips.go 100.00% <0.00%> (+22.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b4eb4a...658148a. Read the comment docs.

@anshlykov
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@anshlykov anshlykov marked this pull request as ready for review August 28, 2020 15:36
@anshlykov anshlykov requested a review from a team as a code owner August 28, 2020 15:36
@anshlykov anshlykov requested a review from gsquared94 August 28, 2020 15:36
@@ -36,13 +37,16 @@ func NewCmdRun() *cobra.Command {
WithExample("Build, test, deploy and tail the logs", "run --tail").
WithExample("Run with a given profile", "run -p <profile>").
WithCommonFlags().
WithFlags(func(f *pflag.FlagSet) {
f.StringSliceVarP(&opts.TargetImages, "build-image", "b", nil, "Choose which artifacts to build. Artifacts with image names that contain the expression will be built only. Default is to build sources for all artifacts")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
f.StringSliceVarP(&opts.TargetImages, "build-image", "b", nil, "Choose which artifacts to build. Artifacts with image names that contain the expression will be built only. Default is to build sources for all artifacts")
f.StringSliceVarP(&opts.TargetImages, "build-image", "b", nil, "Only build artifacts with image names that contain the given substring. Default is to build sources for all artifacts")

nit: I know that you took the same text as the existing build-image flag on build. But the grammar seems off.

Copy link
Member

Choose a reason for hiding this comment

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

If the flag is the same elsewhere then we should instead add it to the flagRegistry.

Copy link
Contributor

@gsquared94 gsquared94 left a comment

Choose a reason for hiding this comment

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

LGTM

@gsquared94 gsquared94 added the kokoro:run runs the kokoro jobs on a PR label Sep 10, 2020
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Sep 10, 2020
Comment on lines 77 to 88
{
ImageName: "first",
},
{
ImageName: "second-test",
},
{
ImageName: "test",
},
{
ImageName: "aaabbbccc",
},
Copy link
Member

Choose a reason for hiding this comment

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

These take up a lot of space!

Suggested change
{
ImageName: "first",
},
{
ImageName: "second-test",
},
{
ImageName: "test",
},
{
ImageName: "aaabbbccc",
},
{ImageName: "first"},
{ImageName: "second-test"},
{ImageName: "test"},
{ImageName: "aaabbbccc"},

@gsquared94
Copy link
Contributor

@anshlykov thanks for making all the suggested changes. Can you sync with the master branch; then we can merge this and call it done 👍

@gsquared94 gsquared94 added the kokoro:force-run forces a kokoro re-run on a PR label Sep 18, 2020
@kokoro-team kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Sep 18, 2020
@gsquared94
Copy link
Contributor

Travis bug is showing duplicate checks and stuck on one of them. This PR's actually cleared all checks. Merging.

@gsquared94 gsquared94 merged commit eb86b7f into GoogleContainerTools:master Sep 21, 2020
@anshlykov anshlykov deleted the gh-4496 branch September 21, 2020 10:53
@anshlykov
Copy link
Contributor Author

@gsquared94 thanks for resolving the merge conflict

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option to skaffold run similar to "skaffold build -b"
5 participants