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

Add Docker config validation #185

Closed
wants to merge 1 commit into from
Closed

Add Docker config validation #185

wants to merge 1 commit into from

Conversation

gabrielhartmann
Copy link
Contributor

Remove fallback logic to nanos on violation of CFS bandwidth configuration. Fail fast instead. This change also moves code for configuration out of the large docker.go file.

"time"
)

const MinCfsBandwidth = 1000
Copy link
Contributor

Choose a reason for hiding this comment

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

use const (
MinCfsBandwidth = 1000
MaxCfsBandwidth = 1000000
)

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, it appears these aren't used outside of this package. they should be locally scoped.

executor/runtime/docker/config.go Show resolved Hide resolved
executor/runtime/docker/config.go Show resolved Hide resolved
package docker

import (
"github.com/stretchr/testify/assert"
Copy link
Contributor

Choose a reason for hiding this comment

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

goimports

@@ -4,11 +4,11 @@ import (
"bytes"
"context"
"errors"
"github.com/Netflix/titus-executor/properties"
Copy link
Contributor

Choose a reason for hiding this comment

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

goimports.

@codecov
Copy link

codecov bot commented Oct 4, 2018

Codecov Report

Merging #185 into master will increase coverage by 9.87%.
The diff coverage is 97.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #185      +/-   ##
==========================================
+ Coverage   21.62%   31.49%   +9.87%     
==========================================
  Files          66       67       +1     
  Lines        8393     8397       +4     
==========================================
+ Hits         1815     2645     +830     
+ Misses       6365     5433     -932     
- Partials      213      319     +106
Impacted Files Coverage Δ
executor/runtime/docker/docker.go 47.1% <0%> (+38.82%) ⬆️
executor/runtime/docker/config.go 100% <100%> (ø)
filesystems/watcher.go 62% <0%> (+3.71%) ⬆️
config/config.go 97.1% <0%> (+4.34%) ⬆️
uploader/copy.go 50% <0%> (+5%) ⬆️
api/netflix/titus/agent.pb.go 22.35% <0%> (+5.63%) ⬆️
executor/runtime/docker/capabilities.go 88.88% <0%> (+11.11%) ⬆️
executor/runner/runner.go 60.62% <0%> (+11.87%) ⬆️
executor/runtime/container.go 91.3% <0%> (+13.04%) ⬆️
... and 10 more

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2212

  • 76 of 93 (81.72%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 22.897%

Changes Missing Coverage Covered Lines Changed/Added Lines %
executor/runtime/docker/docker.go 0 3 0.0%
executor/runtime/docker/config.go 76 90 84.44%
Totals Coverage Status
Change from base Build 2197: 0.06%
Covered Lines: 2458
Relevant Lines: 10735

💛 - Coveralls

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

Successfully merging this pull request may close these issues.

3 participants