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

Remove duplication in eventing #1829

Merged

Conversation

dgageot
Copy link
Contributor

@dgageot dgageot commented Mar 19, 2019

Signed-off-by: David Gageot david@gageot.net

@codecov-io
Copy link

Codecov Report

Merging #1829 into master will decrease coverage by 0.5%.
The diff coverage is 22.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1829      +/-   ##
==========================================
- Coverage   45.42%   44.92%   -0.51%     
==========================================
  Files         143      143              
  Lines        6683     6558     -125     
==========================================
- Hits         3036     2946      -90     
+ Misses       3341     3306      -35     
  Partials      306      306
Impacted Files Coverage Δ
pkg/skaffold/kubernetes/port_forward.go 46.31% <0%> (+4.41%) ⬆️
pkg/skaffold/event/event.go 24.67% <0%> (-7.53%) ⬇️
pkg/skaffold/deploy/kustomize.go 32.53% <0%> (+10.57%) ⬆️
pkg/skaffold/build/parallel.go 89.79% <100%> (-3.88%) ⬇️
pkg/skaffold/build/sequence.go 100% <100%> (ø) ⬆️
pkg/skaffold/deploy/helm.go 61.92% <100%> (-2.81%) ⬇️
pkg/skaffold/deploy/kubectl.go 74.24% <50%> (+10.09%) ⬆️

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 f6c7f56...e79f2ec. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Mar 19, 2019

Codecov Report

Merging #1829 into master will increase coverage by 0.66%.
The diff coverage is 67.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1829      +/-   ##
==========================================
+ Coverage   45.42%   46.09%   +0.66%     
==========================================
  Files         143      143              
  Lines        6683     6558     -125     
==========================================
- Hits         3036     3023      -13     
+ Misses       3341     3228     -113     
- Partials      306      307       +1
Impacted Files Coverage Δ
pkg/skaffold/event/server.go 0% <0%> (ø) ⬆️
pkg/skaffold/kubernetes/port_forward.go 46.31% <0%> (+4.41%) ⬆️
pkg/skaffold/deploy/kustomize.go 32.53% <0%> (+10.57%) ⬆️
pkg/skaffold/build/parallel.go 89.79% <100%> (-3.88%) ⬇️
pkg/skaffold/build/sequence.go 100% <100%> (ø) ⬆️
pkg/skaffold/deploy/helm.go 61.92% <100%> (-2.81%) ⬇️
pkg/skaffold/deploy/kubectl.go 74.24% <50%> (+10.09%) ⬆️
pkg/skaffold/event/event.go 74.67% <76.47%> (+42.47%) ⬆️

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 f6c7f56...ea1b9bf. Read the comment docs.

And increase test coverage

Signed-off-by: David Gageot <david@gageot.net>
@dgageot dgageot force-pushed the remove-eventing-duplication branch from e79f2ec to ea1b9bf Compare March 19, 2019 13:00
Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

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

LGTM. originally I had designed it this way and there were concerns that there would be too many different events to keep adding individual functions for each, but I think the space of events we plan on supporting is finite and relatively small so this is a lot more clear.

@dgageot dgageot merged commit 9dbfc83 into GoogleContainerTools:master Mar 19, 2019
@nkubala nkubala mentioned this pull request Mar 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants