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

tlsutil cleanup plus added tests #1299

Merged
merged 4 commits into from May 13, 2018

Conversation

Projects
None yet
4 participants
@pete911
Contributor

pete911 commented May 11, 2018

Started with tlsutil - adding tests and cleaning up.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io May 11, 2018

Codecov Report

Merging #1299 into master will increase coverage by 0.85%.
The diff coverage is 65.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1299      +/-   ##
==========================================
+ Coverage   36.94%   37.79%   +0.85%     
==========================================
  Files          64       67       +3     
  Lines        3990     4231     +241     
==========================================
+ Hits         1474     1599     +125     
- Misses       2299     2408     +109     
- Partials      217      224       +7
Impacted Files Coverage Δ
core/controlplane/cluster/cluster.go 49.06% <0%> (-0.25%) ⬇️
tlsutil/x509.go 15.05% <0%> (ø)
tlsutil/pem.go 61.53% <100%> (ø)
tlscerts/cert.go 66.1% <66.1%> (ø)
core/controlplane/config/encrypted_assets.go 71.45% <92.85%> (+0.61%) ⬆️
core/nodepool/cluster/cluster.go 44.8% <0%> (-0.59%) ⬇️
core/nodepool/cluster/factory.go
... and 5 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 17a6430...8fc2979. Read the comment docs.

codecov-io commented May 11, 2018

Codecov Report

Merging #1299 into master will increase coverage by 0.85%.
The diff coverage is 65.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1299      +/-   ##
==========================================
+ Coverage   36.94%   37.79%   +0.85%     
==========================================
  Files          64       67       +3     
  Lines        3990     4231     +241     
==========================================
+ Hits         1474     1599     +125     
- Misses       2299     2408     +109     
- Partials      217      224       +7
Impacted Files Coverage Δ
core/controlplane/cluster/cluster.go 49.06% <0%> (-0.25%) ⬇️
tlsutil/x509.go 15.05% <0%> (ø)
tlsutil/pem.go 61.53% <100%> (ø)
tlscerts/cert.go 66.1% <66.1%> (ø)
core/controlplane/config/encrypted_assets.go 71.45% <92.85%> (+0.61%) ⬆️
core/nodepool/cluster/cluster.go 44.8% <0%> (-0.59%) ⬇️
core/nodepool/cluster/factory.go
... and 5 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 17a6430...8fc2979. Read the comment docs.

Show outdated Hide outdated tlsutil/cert.go
return nil, err
}
for _, cert := range certs {
if cert.IsExpired() {
return nil, fmt.Errorf("the following certificate in file %s has expired:-\n\n%s", path, cert)

This comment has been minimized.

@mumoshu

mumoshu May 13, 2018

Collaborator

Seems like we miss the useful info originally included in the error in CheckAllCertsValid?

@mumoshu

mumoshu May 13, 2018

Collaborator

Seems like we miss the useful info originally included in the error in CheckAllCertsValid?

This comment has been minimized.

@pete911

pete911 May 13, 2018

Contributor

certificate has String() method that prints all the info about certificate (should be the same as before). This method is used in show certificates command as well

@pete911

pete911 May 13, 2018

Contributor

certificate has String() method that prints all the info about certificate (should be the same as before). This method is used in show certificates command as well

This comment has been minimized.

@mumoshu

mumoshu May 13, 2018

Collaborator

Ah! Awesome. I missed that. LGTM 👍

@mumoshu

mumoshu May 13, 2018

Collaborator

Ah! Awesome. I missed that. LGTM 👍

@@ -482,12 +482,18 @@ func ReadRawAssets(dirname string, manageCertificates bool, caKeyRequiredOnContr
path := filepath.Join(dirname, file.name)
data, err := RawCredentialFileFromPath(path, file.defaultValue)
if err != nil {
return nil, fmt.Errorf("Error reading credential file %s: %v", path, err)
return nil, fmt.Errorf("error reading credential file %s: %v", path, err)

This comment has been minimized.

@mumoshu

mumoshu May 13, 2018

Collaborator

Yep, an error message should start with a lower case character! Thx for starting to conform that.

@mumoshu

mumoshu May 13, 2018

Collaborator

Yep, an error message should start with a lower case character! Thx for starting to conform that.

@mumoshu

This comment has been minimized.

Show comment
Hide comment
@mumoshu

mumoshu May 13, 2018

Collaborator

@pete911 Thank you so much for your efforts!
Just finished the first-pass review. WDYT?

Collaborator

mumoshu commented May 13, 2018

@pete911 Thank you so much for your efforts!
Just finished the first-pass review. WDYT?

@k8s-ci-robot k8s-ci-robot added size/XL and removed size/L labels May 13, 2018

@pete911

This comment has been minimized.

Show comment
Hide comment
@pete911

pete911 May 13, 2018

Contributor

ok, all the comments should be done

Contributor

pete911 commented May 13, 2018

ok, all the comments should be done

@mumoshu

This comment has been minimized.

Show comment
Hide comment
@mumoshu

mumoshu May 13, 2018

Collaborator

@pete911 Wow, that was quick! LGTM. I'll merge this once the build passes.

Collaborator

mumoshu commented May 13, 2018

@pete911 Wow, that was quick! LGTM. I'll merge this once the build passes.

@mumoshu mumoshu merged commit da7c90b into kubernetes-incubator:master May 13, 2018

2 checks passed

cla/linuxfoundation pete911 authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mumoshu mumoshu added this to the v0.11.0 milestone May 13, 2018

@pete911 pete911 deleted the HotelsDotCom:tlsutil-cleanup branch May 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment