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

Fix handling of SYNAPSE_NO_TLS in docker image #5005

Merged
merged 13 commits into from Apr 25, 2019

Conversation

4 participants
@f35f0ef9d0e827dae86552d3899f78fc
Copy link
Contributor

commented Apr 4, 2019

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file
  • Pull request includes a sign off

f35f0ef9d0e827dae86552d3899f78fc added some commits Apr 2, 2019

Revert "Updated changelog"
This reverts commit ccaa32c.
Revert "Fixing #4663"
This reverts commit ceeae4d.
@f35f0ef9d0e827dae86552d3899f78fc

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

f35f0ef9d0e827dae86552d3899f78fc added some commits Apr 4, 2019

Revert "Add changelog file"
This reverts commit 3109620.
@codecov

This comment has been minimized.

Copy link

commented Apr 4, 2019

Codecov Report

Merging #5005 into develop will decrease coverage by 0.12%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #5005      +/-   ##
===========================================
- Coverage     60.7%   60.57%   -0.13%     
===========================================
  Files          332      331       -1     
  Lines        34172    34213      +41     
  Branches      5633     5655      +22     
===========================================
- Hits         20743    20725      -18     
- Misses       11958    12013      +55     
- Partials      1471     1475       +4
@codecov

This comment has been minimized.

Copy link

commented Apr 4, 2019

Codecov Report

Merging #5005 into develop will decrease coverage by 0.13%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #5005      +/-   ##
===========================================
- Coverage     60.7%   60.57%   -0.14%     
===========================================
  Files          332      331       -1     
  Lines        34172    34213      +41     
  Branches      5633     5655      +22     
===========================================
- Hits         20743    20723      -20     
- Misses       11958    12015      +57     
- Partials      1471     1475       +4

@f35f0ef9d0e827dae86552d3899f78fc f35f0ef9d0e827dae86552d3899f78fc marked this pull request as ready for review Apr 4, 2019

@babolivier

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

For link: #4663

Show resolved Hide resolved docker/start.py Outdated

@erikjohnston erikjohnston added this to In progress in Homeserver Task Board via automation Apr 4, 2019

@richvdh richvdh changed the title Fix4663 Fix handling of SYNAPSE_NO_TLS in docker image Apr 4, 2019

@richvdh richvdh self-requested a review Apr 24, 2019

@richvdh
Copy link
Member

left a comment

lgtm! thanks. I wrote a couple of thoughts below which I hope you might find useful for future reference.

Another thought is that it would have been nice to factor out some of the logic to a separate function, so that we could use it for other boolean parameters like SYNAPSE_ALLOW_GUEST and SYNAPSE_REPORT_STATS. We can make that change another time though.


# Convert SYNAPSE_NO_TLS to boolean if exists
if "SYNAPSE_NO_TLS" in environ:
tlsanswerstring = str.lower(environ["SYNAPSE_NO_TLS"])

This comment has been minimized.

Copy link
@richvdh

richvdh Apr 25, 2019

Member

might have been nice to write tlsanswerstring = str.lower(environ.get("SYNAPSE_NO_TLS", "false")) here, to avoid the if condition above.

if tlsanswerstring in ("true", "on", "1", "yes"):
environ["SYNAPSE_NO_TLS"] = True
else:
if tlsanswerstring in ("false", "off", "0", "no"):

This comment has been minimized.

Copy link
@richvdh

richvdh Apr 25, 2019

Member

would have been nice to use elif rather than another layer of nesting.

@richvdh

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

test failures are due to #5096

@richvdh richvdh merged commit 4a9a118 into matrix-org:develop Apr 25, 2019

9 of 22 checks passed

buildkite/synapse Build #1124 failed (3 minutes, 57 seconds)
Details
buildkite/synapse/check-sample-config Failed (exit status 1)
Details
buildkite/synapse/python-2-dot-7-slash-postgres-9-dot-4 Failed (exit status 1)
Details
buildkite/synapse/python-2-dot-7-slash-postgres-9-dot-5 Failed (exit status 1)
Details
buildkite/synapse/python-2-dot-7-slash-sqlite Failed (exit status 1)
Details
buildkite/synapse/python-2-dot-7-slash-sqlite-slash-old-deps Failed (exit status 1)
Details
buildkite/synapse/python-3-dot-5-slash-postgres-9-dot-4 Failed (exit status 1)
Details
buildkite/synapse/python-3-dot-5-slash-postgres-9-dot-5 Failed (exit status 1)
Details
buildkite/synapse/python-3-dot-5-slash-sqlite Failed (exit status 1)
Details
buildkite/synapse/python-3-dot-6-slash-sqlite Failed (exit status 1)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-11 Failed (exit status 1)
Details
buildkite/synapse/python-3-dot-7-slash-postgres-9-dot-5 Failed (exit status 1)
Details
buildkite/synapse/python-3-dot-7-slash-sqlite Failed (exit status 1)
Details
buildkite/synapse/isort Passed (15 seconds)
Details
buildkite/synapse/newspaper-newsfile Passed (13 seconds)
Details
buildkite/synapse/packaging Passed (19 seconds)
Details
buildkite/synapse/pep-8 Passed (50 seconds)
Details
buildkite/synapse/pipeline Passed (4 seconds)
Details
ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details

Homeserver Task Board automation moved this from In progress to Done Apr 25, 2019

anoadragon453 added a commit that referenced this pull request Apr 30, 2019

Merge branch 'develop' into anoa/blacklist_ip_ranges
* develop: (34 commits)
  Add a default .m.rule.tombstone push rule (#4867)
  Fix infinite loop in presence handler
  changelog
  more logging improvements
  remove extraneous exception logging
  Clarify logging when PDU signature checking fails
  Changelog
  Add --no-pep-517 to README instructions
  set PIP_USE_PEP517 = False for tests
  Fix handling of SYNAPSE_NO_TLS in docker image (#5005)
  Config option for verifying federation certificates (MSC 1711) (#4967)
  Remove log error for .well-known/matrix/client (#4972)
  Prevent "producer not unregistered" message (#5009)
  add gpg key fingerprint
  Don't crash on lack of expiry templates
  Update debian install docs for new key and repo (#5074)
  Add management endpoints for account validity
  Send out emails with links to extend an account's validity period
  Make sure we're not registering the same 3pid twice
  Newsfile
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.