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

Fix config reload in skaffold dev #2279

Merged
merged 3 commits into from
Jun 20, 2019

Conversation

dgageot
Copy link
Contributor

@dgageot dgageot commented Jun 18, 2019

Fix #2278 and #2281

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

@dgageot dgageot marked this pull request as ready for review June 18, 2019 11:42
@dgageot dgageot added the kokoro:run runs the kokoro jobs on a PR label Jun 18, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jun 18, 2019
@dgageot dgageot force-pushed the fix-2278 branch 2 times, most recently from d89cca8 to 6d0d63b Compare June 19, 2019 12:15
dgageot added 3 commits June 19, 2019 15:29
Fix GoogleContainerTools#2278

Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
And only stop it at the end.
Fix GoogleContainerTools#2281

Signed-off-by: David Gageot <david@gageot.net>
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.

thanks, I think I misinterpreted what the dev loop was doing when the skaffold.yaml was changed.

do you want to just remove the build trigger here? since I'm working on some bigger changes that are going to change the way the build trigger is handled anyway, I'm not opposed to removing it until those changes are in

@dgageot
Copy link
Contributor Author

dgageot commented Jun 19, 2019

Isn’t it used? If it’s not, I’ll update the PR tomorrow to remove it. Or maybe we can merge this one and remove the trigger in another PR

@nkubala
Copy link
Contributor

nkubala commented Jun 19, 2019

it will be used (eventually) if it's not right now, but it might not be yet. we should ask the IDE teams. I'm just not super happy with the current implementation but I suppose it's not doing a ton of harm leaving it in for now

@dgageot
Copy link
Contributor Author

dgageot commented Jun 19, 2019

Let’s keep it here then and wait for you to change it

@codecov
Copy link

codecov bot commented Jun 19, 2019

Codecov Report

Merging #2279 into master will decrease coverage by 0.01%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2279      +/-   ##
==========================================
- Coverage   60.94%   60.93%   -0.02%     
==========================================
  Files         186      186              
  Lines        7877     7871       -6     
==========================================
- Hits         4801     4796       -5     
- Misses       2690     2691       +1     
+ Partials      386      384       -2
Impacted Files Coverage Δ
pkg/skaffold/runner/context/context.go 62.5% <ø> (-1.14%) ⬇️
cmd/skaffold/app/cmd/dev.go 23.8% <0%> (+1.58%) ⬆️
pkg/skaffold/runner/dev.go 56.52% <0%> (ø) ⬆️
pkg/skaffold/runner/runner.go 70.73% <100%> (ø) ⬆️
pkg/skaffold/server/server.go 53.22% <100%> (-1.47%) ⬇️
pkg/skaffold/watch/triggers.go 45.65% <100%> (ø) ⬆️
pkg/skaffold/runner/deploy.go 28.57% <0%> (-14.29%) ⬇️

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 9073754...c12b56d. Read the comment docs.

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.

alright, I'll address the build trigger stuff in a separate PR. this is fine for now, thanks for fixing it.

also, go to sleep 😪

@dgageot dgageot merged commit ee24699 into GoogleContainerTools:master Jun 20, 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.

Changing skaffold.yaml stops skaffold dev
4 participants