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

bug/1560 tlsCipherSuites and tlsVersion marshaling #1603

Merged
merged 2 commits into from
Sep 2, 2020

Conversation

berndhartzer
Copy link
Contributor

Closes #1560

@berndhartzer
Copy link
Contributor Author

The linter picks up many errors, most of which are for files I haven't changed. One error that is relevant PR is this one:

lib/options.go:101:1                        golint            exported method `TLSCipherSuites.MarshalJSON` should have comment or be unexported

But this line is similar to others in the file (e.g. line 49: func (v TLSVersion) MarshalJSON() ([]byte, error) {) so I'm not sure how to go about resolving that error (or the other errors), let me know and I can amend the PR!

@mstoykov
Copy link
Contributor

Hi @berndhartzer, thanks for the PR !!! 🎉

We have fixed the lint to not get wrong files (or at least for your case) so please just rebase on top of the current master and force-push in order to get the fix

@berndhartzer berndhartzer force-pushed the bug/1560-tls-marshaling branch from d8ceb1a to aee477a Compare August 24, 2020 07:03
@berndhartzer berndhartzer force-pushed the bug/1560-tls-marshaling branch from aee477a to 576c7b9 Compare August 24, 2020 07:20
@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2020

Codecov Report

Merging #1603 into master will increase coverage by 0.01%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1603      +/-   ##
==========================================
+ Coverage   74.82%   74.83%   +0.01%     
==========================================
  Files         164      164              
  Lines       14035    14043       +8     
==========================================
+ Hits        10501    10509       +8     
+ Misses       3002     3001       -1     
- Partials      532      533       +1     
Impacted Files Coverage Δ
lib/options.go 87.19% <75.00%> (-0.51%) ⬇️
lib/testutils/minirunner/minirunner.go 78.57% <0.00%> (-3.58%) ⬇️
lib/executor/vu_handle.go 95.49% <0.00%> (+1.80%) ⬆️
lib/executors.go 94.11% <0.00%> (+1.96%) ⬆️

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 c68a4eb...9160ea8. Read the comment docs.

@berndhartzer
Copy link
Contributor Author

Hi @berndhartzer, thanks for the PR !!! 🎉

We have fixed the lint to not get wrong files (or at least for your case) so please just rebase on top of the current master and force-push in order to get the fix

Thanks @mstoykov I've fixed it now 🙂

@mstoykov mstoykov requested a review from na-- September 1, 2020 07:22
mstoykov
mstoykov previously approved these changes Sep 2, 2020
imiric
imiric previously approved these changes Sep 2, 2020
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

LGTM, just a nitpick and a comment.

lib/options.go Outdated Show resolved Hide resolved
@@ -235,7 +250,7 @@ type Options struct {

// Specify TLS versions and cipher suites, and present client certificates.
TLSCipherSuites *TLSCipherSuites `json:"tlsCipherSuites" envconfig:"K6_TLS_CIPHER_SUITES"`
TLSVersion *TLSVersions `json:"tlsVersion" envconfig:"K6_TLS_VERSION"`
TLSVersion *TLSVersions `json:"tlsVersion" ignored:"true"`
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I think we could fix the envconfig issue with a custom decoder and keep the K6_TLS_VERSION env var, but since it's not documented in the k6 docs, and making it work is tricky in general because of #883, I'm OK with ignoring it.

Copy link
Member

@na-- na-- Sep 2, 2020

Choose a reason for hiding this comment

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

We can't keep delaying the envconfig removal, so lets ignore this for now and fix it properly everywhere soon.

@na-- na-- added this to the v0.28.0 milestone Sep 2, 2020
@berndhartzer berndhartzer dismissed stale reviews from imiric and mstoykov via 9160ea8 September 2, 2020 09:42
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

Thanks for your work!

@na-- na-- merged commit 644bfd2 into grafana:master Sep 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tlsCipherSuites and tlsVersion aren't properly marshalled in archives
5 participants