Skip to content
This repository has been archived by the owner on Jan 10, 2023. It is now read-only.

Terminate during prepare #119

Merged
merged 2 commits into from
May 15, 2018
Merged

Terminate during prepare #119

merged 2 commits into from
May 15, 2018

Conversation

sargun
Copy link
Contributor

@sargun sargun commented May 15, 2018

No description provided.

@codecov
Copy link

codecov bot commented May 15, 2018

Codecov Report

Merging #119 into master will increase coverage by 0.13%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
+ Coverage   35.55%   35.69%   +0.13%     
==========================================
  Files          65       65              
  Lines        7341     7349       +8     
==========================================
+ Hits         2610     2623      +13     
+ Misses       4430     4425       -5     
  Partials      301      301
Impacted Files Coverage Δ
executor/runner/runner.go 64.9% <90%> (+1.84%) ⬆️
executor/drivers/titusdriver.go 87.5% <0%> (+12.5%) ⬆️

@sargun sargun force-pushed the terminate-during-prepare branch 2 times, most recently from 8cd092e to 8d75def Compare May 15, 2018 04:33
@coveralls
Copy link

coveralls commented May 15, 2018

Pull Request Test Coverage Report for Build 907

  • 9 of 10 (90.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.2%) to 25.961%

Changes Missing Coverage Covered Lines Changed/Added Lines %
executor/runner/runner.go 9 10 90.0%
Totals Coverage Status
Change from base Build 854: 0.2%
Covered Lines: 2411
Relevant Lines: 9287

💛 - Coveralls

@sargun sargun requested review from fabiokung and andrew-leung May 15, 2018 05:04
@sargun
Copy link
Contributor Author

sargun commented May 15, 2018

The code is still very much scratch now, but what do you think -- if we get a kill, should we cancel the prepare context (and therefore the image download)?

@sargun sargun force-pushed the terminate-during-prepare branch from 6b23ba9 to e7e59af Compare May 15, 2018 18:15
@sargun
Copy link
Contributor Author

sargun commented May 15, 2018

@fabiokung @andrew-leung This should be ready to look at.

@@ -260,10 +260,19 @@ no_launchguard:
}
r.updateStatus(ctx, titusdriver.Starting, "creating")

prepareCtx, prepareCancel := context.WithCancel(ctx)
defer prepareCancel()
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: I think this can be moved to a single prepareCancel call right after Prepare(...) (and right before the if err != nil), since it's the only place it needs to be called, regardless if Prepare failed or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

metalinter complains if I do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:(

@sargun sargun merged commit 33b8605 into master May 15, 2018
@sargun sargun deleted the terminate-during-prepare branch May 15, 2018 23:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants