Skip to content

Conversation

@aldas
Copy link
Contributor

@aldas aldas commented Feb 27, 2021

Fix Echo.Serve() will not serve on HTTP port correctly when there is already TLSListener set to Echo instance. (#1785)

This is problem when user tries to use HTTP (e.Listener) and HTTPS (e.TLSListener) with same Echo instance. In this situation StartTLS could have already set TLSListener on echo instance and now Start call will use TLSListener instance on its own server instance - which is wrong.

Introduced with #1735 and affects only latest v4.2.0 version

@codecov
Copy link

codecov bot commented Feb 27, 2021

Codecov Report

Merging #1793 (17adf06) into master (d9e2354) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1793   +/-   ##
=======================================
  Coverage   89.73%   89.73%           
=======================================
  Files          32       32           
  Lines        2669     2669           
=======================================
  Hits         2395     2395           
  Misses        175      175           
  Partials       99       99           
Impacted Files Coverage Δ
echo.go 91.53% <100.00%> (ø)

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 d9e2354...17adf06. Read the comment docs.

…lready TLSListener set to Echo instance. (labstack#1785)

This is problem when user tries to use HTTP (e.Listener) and HTTPS (e.TLSListener) with same Echo instance.
@aldas aldas requested a review from lammel February 28, 2021 17:17
@@ -0,0 +1,8 @@
To Generate valid certificate and private key use following command:
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix typo => To generate a valid certificate and private key use the following command

Copy link
Contributor Author

@aldas aldas Feb 28, 2021

Choose a reason for hiding this comment

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

fixed ... with multiple commits. please use squash when merging.

Copy link
Contributor

@lammel lammel left a comment

Choose a reason for hiding this comment

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

Looks good, added test will ensure both TLS and non-TLS work.
Approved! Thanks @aldas

@lammel lammel merged commit c79ffed into labstack:master Feb 28, 2021
This was referenced Mar 8, 2021
@aldas aldas deleted the fix_issue_1785 branch May 23, 2021 06:07
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.

2 participants