Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

Make containerized deployment more configurable #1880

Merged
merged 2 commits into from
Jul 26, 2018

Conversation

flavio
Copy link
Member

@flavio flavio commented Jul 12, 2018

A bunch of changes required to be more flexible when deploying Portus into a containerized environment.

TODO

@flavio
Copy link
Member Author

flavio commented Jul 12, 2018

I think this should be backported also to the 2.3 branch

docker/init Outdated
@@ -82,7 +82,6 @@ done
update-ca-certificates

# Further settings
export PORTUS_PUMA_HOST="0.0.0.0:3000"
Copy link
Collaborator

@mssola mssola Jul 12, 2018

Choose a reason for hiding this comment

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

I wouldn't delete this, since it will break the deployment for most users. I propose to export this env. variable to this default value if none has been provided.

Copy link
Member Author

@flavio flavio Jul 20, 2018

Choose a reason for hiding this comment

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

This isn't going to help with my use-case: have puma bind to unix socket even when running Portus using the official container, see:

Portus/config/puma.rb

Lines 19 to 31 in 7358f3a

if ENV["PORTUS_PUMA_HOST"]
if ENV["PORTUS_PUMA_TLS_KEY"]
host, port = ENV["PORTUS_PUMA_HOST"].split(":")
port ||= "3000"
ssl_bind host, port,
key: ENV["PORTUS_PUMA_TLS_KEY"],
cert: ENV["PORTUS_PUMA_TLS_CERT"]
else
bind "tcp://#{ENV["PORTUS_PUMA_HOST"]}"
end
else
bind "unix://#{File.join(Dir.pwd, "tmp/sockets/puma.sock")}"
end

Another way would be to rework the code I pasted above to something like:

if ENV["PORTUS_PUMA_HOST"] && !ENV["PORTUS_PUMA_USE_UNIX_SOCKET"]
...
else
# bind to socket
end

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer the code you just pasted instead of the old one because I believe that telling people to always set this env. variable to "0.0.0.0:3000" for the most common use case is confusing. Instead, if you really know what you are doing, you can provide this extra env. variable.

So a 👍 from my side.

Copy link
Collaborator

@mssola mssola Jul 20, 2018

Choose a reason for hiding this comment

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

Oh, and instead of !ENV["PORTUS_PUMA_USE_UNIX_SOCKET"], I think it's safer (and more elegant 🤓) ENV["PORTUS_PUMA_USE_UNIX_SOCKET"].blank?

@flavio flavio force-pushed the make-containerized-deployment-more-configurable branch from 9dd48dc to 23af4bf Compare July 20, 2018 12:38
@flavio
Copy link
Member Author

flavio commented Jul 20, 2018

@mssola I've updated the code but I haven't tested it. AFAIK .blank? is defined by active support, I fear this is not available at that time. Can you double check?

@flavio
Copy link
Member Author

flavio commented Jul 20, 2018

Also, which branch holds the documentation?

@mssola
Copy link
Collaborator

mssola commented Jul 20, 2018

I've updated the code but I haven't tested it. AFAIK .blank? is defined by active support, I fear this is not available at that time. Can you double check?

I've tested it and nothing broke 😄 That's because we are calling puma from within the context of a Rails application (notice, for example, that we later call ActiveRecord directly 😉). So, it should be fine.

Also, which branch holds the documentation?

It's on the site-source branch, because of an issue we had with jekyll and custom plugins.

@mssola
Copy link
Collaborator

mssola commented Jul 20, 2018

As for the failing integration tests, maybe a rebase does the trick, otherwise we'll have to check what's broken.

@flavio flavio force-pushed the make-containerized-deployment-more-configurable branch from 23af4bf to 256430a Compare July 20, 2018 14:19
@flavio
Copy link
Member Author

flavio commented Jul 20, 2018

Rebased

@flavio
Copy link
Member Author

flavio commented Jul 24, 2018

@mssola, the patch is not working. I tested it with a containerized image and I got:

undefined method `blank?' for "true":String

I would change the patch to

if ENV["PORTUS_PUMA_HOST"] && ENV["PORTUS_PUMA_USE_UNIX_SOCKET"] != "true"

this works fine.

@mssola
Copy link
Collaborator

mssola commented Jul 26, 2018

@flavio tomorrow we want to release 2.3.4, which includes an important fix. Could you update this PR so we can also include this there?

Introduce the `PORTUS_PUMA_USE_UNIX_SOCKET`. Setting this environment
variable will cause puma to use a unix socket instead of binding to a
network port.

Signed-off-by: Flavio Castelli <[email protected]>
Allow to instantiate a connection with a database listening over a unix
socket.

Signed-off-by: Flavio Castelli <[email protected]>
@flavio flavio force-pushed the make-containerized-deployment-more-configurable branch from 256430a to ce2f63f Compare July 26, 2018 12:13
@flavio
Copy link
Member Author

flavio commented Jul 26, 2018

@mssola done!

@flavio flavio force-pushed the make-containerized-deployment-more-configurable branch from ce2f63f to 4b57ad6 Compare July 26, 2018 12:14
Copy link
Collaborator

@mssola mssola left a comment

Choose a reason for hiding this comment

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

Thanks 👏

@mssola mssola merged commit e886d51 into master Jul 26, 2018
@mssola mssola deleted the make-containerized-deployment-more-configurable branch July 26, 2018 14:08
@mssola
Copy link
Collaborator

mssola commented Jul 26, 2018

Cherry-picked and pushed into v2.3.

mssola added a commit that referenced this pull request Jul 26, 2018
The pull request #1880 introduced two new environment variables. This
commit updates the documentation explaining both of them.

Signed-off-by: Miquel Sabaté Solà <[email protected]>
mssola added a commit that referenced this pull request Jul 26, 2018
The pull request #1880 introduced two new environment variables. This
commit updates the documentation explaining both of them.

Signed-off-by: Miquel Sabaté Solà <[email protected]>
mssola added a commit that referenced this pull request Jul 26, 2018
docs: updated the documentation for #1880
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants