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

feat(cert): Support for extra TLS options #2258

Merged
merged 7 commits into from
Sep 28, 2023
Merged

Conversation

tfoldi
Copy link
Contributor

@tfoldi tfoldi commented Sep 14, 2023

Support for extra SSL parameters for mqtt and http sources. Fixes #2241

@tfoldi tfoldi changed the title Add support for TLSMinVersion and RenegotiationOptions for mqtt and http feat: Support for TLSMinVersion and RenegotiationOptions Sep 14, 2023
@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (f5e0182) 62.79% compared to head (50b0601) 62.91%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2258      +/-   ##
==========================================
+ Coverage   62.79%   62.91%   +0.12%     
==========================================
  Files         292      292              
  Lines       34995    35029      +34     
==========================================
+ Hits        21972    22037      +65     
+ Misses      10990    10958      -32     
- Partials     2033     2034       +1     
Files Coverage Δ
internal/io/http/client.go 84.30% <100.00%> (+0.11%) ⬆️
internal/pkg/cert/cert.go 71.05% <100.00%> (+18.88%) ⬆️
internal/topo/connection/clients/mqtt/mqtt.go 33.33% <100.00%> (+1.33%) ⬆️

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tfoldi tfoldi changed the title feat: Support for TLSMinVersion and RenegotiationOptions feat(cert): Support for extra TLS oprtions Sep 14, 2023
@tfoldi tfoldi changed the title feat(cert): Support for extra TLS oprtions feat(cert): Support for extra TLS options Sep 14, 2023
@ngjaying ngjaying added this to the 1.12 milestone Sep 19, 2023
@tfoldi tfoldi marked this pull request as ready for review September 24, 2023 03:38
@tfoldi tfoldi requested a review from ngjaying September 24, 2023 03:39
Copy link
Collaborator

@ngjaying ngjaying 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 need to pass markdown check. Additionally, it'll be better to increase ut coverage to pass the condecov check. Thanks.

@tfoldi
Copy link
Contributor Author

tfoldi commented Sep 24, 2023

LGTM. Just need to pass markdown check. Additionally, it'll be better to increase ut coverage to pass the condecov check. Thanks.

The markdown check fails in a file that I did not touch. Actually it is failing due to #2272 committed by @Yisaer two days ago (see https://github.com/lf-edge/ekuiper/actions/runs/6269824039/job/17026819627).

Run markdownlint -c .github/workflows/markdown_config.json ./docs/
  markdownlint -c .github/workflows/markdown_config.json ./docs/
  shell: /usr/bin/bash -e {0}
docs/en_US/configuration/global_configurations.md:[2](https://github.com/lf-edge/ekuiper/actions/runs/6287798441/job/17072623952?pr=2258#step:4:2)[5](https://github.com/lf-edge/ekuiper/actions/runs/6287798441/job/17072623952?pr=2258#step:4:6)9 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2]

Locally, the markdown test passes so "it's not me, it's you" :)

❯ markdownlint -c .github/workflows/markdown_config.json docs
❯ echo $?
0

For tests, sure, I can add tests for those lines.

@ngjaying
Copy link
Collaborator

LGTM. Just need to pass markdown check. Additionally, it'll be better to increase ut coverage to pass the condecov check. Thanks.

The markdown check fails in a file that I did not touch. Actually it is failing due to #2272 committed by @Yisaer two days ago (see https://github.com/lf-edge/ekuiper/actions/runs/6269824039/job/17026819627).

Run markdownlint -c .github/workflows/markdown_config.json ./docs/
  markdownlint -c .github/workflows/markdown_config.json ./docs/
  shell: /usr/bin/bash -e {0}
docs/en_US/configuration/global_configurations.md:[2](https://github.com/lf-edge/ekuiper/actions/runs/6287798441/job/17072623952?pr=2258#step:4:2)[5](https://github.com/lf-edge/ekuiper/actions/runs/6287798441/job/17072623952?pr=2258#step:4:6)9 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2]

Locally, the markdown test passes so "it's not me, it's you" :)

❯ markdownlint -c .github/workflows/markdown_config.json docs
❯ echo $?
0

For tests, sure, I can add tests for those lines.

Ah, it's weird. The markdown check should have run in that PR. Never mind, we'll fix that. Appreciate your patience.

@Yisaer
Copy link
Collaborator

Yisaer commented Sep 26, 2023

plz update the base master branch as we have fixed the markdown lint error

@tfoldi tfoldi requested a review from ngjaying September 26, 2023 16:34
Copy link
Collaborator

@ngjaying ngjaying left a comment

Choose a reason for hiding this comment

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

LGTM. What a long run, thanks for the patience.

@ngjaying ngjaying merged commit d45e587 into lf-edge:master Sep 28, 2023
@tfoldi tfoldi deleted the tls_options branch September 28, 2023 05:10
ngjaying pushed a commit to ngjaying/kuiper that referenced this pull request Oct 9, 2023
ngjaying pushed a commit that referenced this pull request Oct 9, 2023
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.

eKuiper cannot connect to Azure IoT Hub (probably due to missing RenegotiateOnceAsClient)
3 participants