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

Improve runner code #1204

Closed

Conversation

dgageot
Copy link
Contributor

@dgageot dgageot commented Oct 25, 2018

Let's remove some duplication and simplify the runner's code.

@codecov-io
Copy link

codecov-io commented Oct 30, 2018

Codecov Report

Merging #1204 into master will increase coverage by 0.25%.
The diff coverage is 54.6%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1204      +/-   ##
==========================================
+ Coverage   42.24%   42.49%   +0.25%     
==========================================
  Files          96      101       +5     
  Lines        4242     4292      +50     
==========================================
+ Hits         1792     1824      +32     
- Misses       2276     2295      +19     
+ Partials      174      173       -1
Impacted Files Coverage Δ
pkg/skaffold/config/options.go 88.23% <ø> (ø) ⬆️
cmd/skaffold/app/cmd/deploy.go 0% <0%> (ø) ⬆️
pkg/skaffold/kubernetes/log.go 6.77% <0%> (ø) ⬆️
pkg/skaffold/runner/deps.go 0% <0%> (ø)
pkg/skaffold/test/test.go 0% <0%> (ø)
pkg/skaffold/kubernetes/colorpicker.go 100% <100%> (ø) ⬆️
pkg/skaffold/runner/runner.go 56.87% <63.23%> (+7.47%) ⬆️
pkg/skaffold/test/noop.go 66.66% <66.66%> (ø)
pkg/skaffold/build/prebuilt.go 82.75% <82.75%> (ø)
pkg/skaffold/build/parallel.go 0% <0%> (ø)
... and 7 more

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 d1648a8...b85b7bf. Read the comment docs.

@dgageot dgageot force-pushed the improve-runner-code branch 4 times, most recently from 7f0d914 to 745be3f Compare November 6, 2018 16:40
@dgageot
Copy link
Contributor Author

dgageot commented Nov 8, 2018

ping @GoogleContainerTools/container-tools can you please take a look. I tried to remove a lot of duplication in the runner code. It was becoming a bit too complicated to maintain.
It's now super simple yet but there should be less room for mistakes.

Copy link
Contributor

@r2d4 r2d4 left a comment

Choose a reason for hiding this comment

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

Can you split out the error text changes as a new PR?

Both commands should follow the same path
except `deploy` will work with pre-built
images.

Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
@dgageot
Copy link
Contributor Author

dgageot commented Nov 8, 2018

@r2d4 done in #1255

imageList := kubernetes.NewImageList()
for _, b := range bRes {
imageList.Add(b.Tag)
if err = r.Test(ctx, out, bRes); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about skip if test is nil and get rid of noop tester?

Copy link
Contributor

Choose a reason for hiding this comment

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

And the helper function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if tests is nil? Do you mean we'd have to store the SkaffoldPipeline in the SkaffoldRunner instance?

}

func (b *prebuiltImagesBuilder) Build(ctx context.Context, out io.Writer, tagger tag.Tagger, artifacts []*latest.Artifact) ([]Artifact, error) {
tags := make(map[string]string)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we’re already here, how about we generate the git tags if no images were supplied?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can also resolve the taggers from the yams if you want to be really correct (to make sure we can generate reproducibly)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry @r2d4, I don't understand

Copy link
Contributor

@r2d4 r2d4 Nov 8, 2018

Choose a reason for hiding this comment

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

If images isn't provided, try to seed it by using the passed in tagger to generate a tag for each artifact

if images == nil {
  for _, a := range artifacts { 
    tag, err := tag.GenerateFullyQualifiedTag(a.Image, nil)
    if err != nil // this is the case where we ended up with a tagger that needs some options passed in, fail
    images = append(images, tag)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

which would fix #653

}, nil
}

func getBuilder(cfg *latest.BuildConfig, kubeContext string) (build.Builder, error) {
func getBuilder(cfg *latest.BuildConfig, kubeContext string, opts *config.SkaffoldOptions) (build.Builder, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Slightly weird but I get it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I know

@dgageot dgageot closed this Nov 28, 2018
@dgageot dgageot deleted the improve-runner-code branch December 28, 2018 07:14
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.

3 participants