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

Connection ID fallback and Docker Compose support #209

Merged
merged 11 commits into from
Oct 22, 2019

Conversation

ryanrolds
Copy link
Contributor

@ryanrolds ryanrolds commented Oct 21, 2019

I put the ID fallback in fdcache because the m-lab/uuid appears to function as a wrapper for getting an ID via the TCP connections SO_COOKIE and I don't think it makes sense to muddy that package with fallback support.

The PR also adds support for Docker Compose. The Dockerfile.local is used by docker-compose.yml because the main Dockerfile doesn't copy gen_local_test_certs.sh into the image. I also switch gen_local_test_certs.sh to use /bin/sh as I'm pretty sure Alpine Linux doesn't include Bash. I also added the cert moving into the script.


This change is Reviewable

@coveralls
Copy link
Collaborator

coveralls commented Oct 22, 2019

Pull Request Test Coverage Report for Build 1050

  • 5 of 14 (35.71%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.3%) to 79.162%

Changes Missing Coverage Covered Lines Changed/Added Lines %
fdcache/fdcache.go 5 14 35.71%
Totals Coverage Status
Change from base Build 1048: -0.3%
Covered Lines: 1569
Relevant Lines: 1982

💛 - Coveralls

Copy link
Contributor

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

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

This looks good to me, except a doubt I have with respect to the script for generating certs being compatible with non-bash shells, such as ksh and dash. Could you please check that?

@@ -1,6 +1,7 @@
#!/bin/bash
#!/bin/sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you test whether this works with dash and ksh? I believe the main reason why it was named bash and it was invoking explicitly bash is because it is using the non-portable set -euxo pipefail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're correct, it doesn't work under Dash. I'm switching the interpreter back to Bash and updating Dockerfile.local to install Bash.

Copy link
Contributor

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

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

🐳

Thank you for putting this together!

@bassosimone
Copy link
Contributor

Will merge when Travis becomes green

@bassosimone bassosimone merged commit db84d15 into m-lab:master Oct 22, 2019
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.

3 participants