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/review setup (#52) #54

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Fix/review setup (#52) #54

wants to merge 9 commits into from

Conversation

dkastl
Copy link
Member

@dkastl dkastl commented Sep 15, 2022

@sanak , this is my suggestion for a simplified Docker setup.
It can be improved of course.


Resolve #52

Signed-off-by: Daniel Kastl <[email protected]>
Signed-off-by: Daniel Kastl <[email protected]>
@sanak
Copy link
Member

sanak commented Sep 15, 2022

@dkastl Okay, thanks!
I will add some comments with comparing PR #53 - feature/simplify-docker-settings branch.

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
gem "pg", (ENV['GEM_PG_VERSION'] ? "~> #{ENV['GEM_PG_VERSION']}" : "~> 1.2.3")
gem 'puma'
gem 'puma_worker_killer'
gem 'lograge'
Copy link
Member

@sanak sanak Sep 15, 2022

Choose a reason for hiding this comment

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

@dkastl (CC: @smellman)
In my opinion, puma_worker_killer is for production environment, but shall we assume this docker-gtt setup for production environment (not for development environment) ?

Also, lograge is for changing rails log as JSON format which is convenience on AWS ECS log and CloudWatch, but shall we assume AWS as first deployment priority ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure ... but we actually run it --without test development and with production environment, so I wouldn't say it's a problem to keep it.

Copy link
Member

Choose a reason for hiding this comment

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

@dkastl

but we actually run it --without test development and with production environment,

Okay, sure at this moment...
By the way, I found the way to support both (production and test development) env in another project, so it can be solved.

For debugging purpose, multiple workers (=3) and puma_worker_killer is quite annoying for me, so personally configurable this part with additional environment variables will be the best way.

Also, lograge is for changing rails log as JSON format which is convenience on AWS ECS log and CloudWatch, but shall we assume AWS as first deployment priority ?

How about this ?

Copy link
Member

Choose a reason for hiding this comment

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

By the way, mailcatcher is used in docker-compose.yml, so it's a kind of half production and half development env.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, don't know very much about these details. Could you just make a change that works good for development and push it to this branch?

Copy link
Member

@sanak sanak Sep 15, 2022

Choose a reason for hiding this comment

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

Well, don't know very much about these details. Could you just make a change that works good for development and push it to this branch?

Do you mean only the following part ?

By the way, I found the way to support both (production and test development) env in another project, so it can be solved.

If so, it's okay for me.

But if it's also gem dependencies (puma, puma_worker_killer and lograge), we need to decide something this project's scope ,etc ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the question is, what the "target audience" of this Docker setup is. I think your perspective is the developer point of view. But there could be also the point of view of someone, who wants to use this to deploy and use the service. For example some SMASH user.

Of course it's best to support both use cases, for example with ENV settings. But we don't need to have this all ready with the first PR.

Maybe the non-developer use case should be priority, because a developer can also adjust the Docker setup. What do you think?

config/configuration.yml Outdated Show resolved Hide resolved
.env.example Outdated Show resolved Hide resolved
config/configuration.yml Outdated Show resolved Hide resolved
config/configuration.yml Outdated Show resolved Hide resolved
config/puma.rb Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
@sanak sanak mentioned this pull request Sep 15, 2022
@sanak sanak added the enhancement New feature or request label Sep 15, 2022
@sanak sanak changed the title Fix/review setup Fix/review setup (#52) Sep 15, 2022
Signed-off-by: Daniel Kastl <[email protected]>
Signed-off-by: Daniel Kastl <[email protected]>
Signed-off-by: Daniel Kastl <[email protected]>
Signed-off-by: Daniel Kastl <[email protected]>
Signed-off-by: Daniel Kastl <[email protected]>
domain: <%= ENV['SMTP_DOMAIN'] %>
authentication: <%= ENV['SMTP_AUTHENTICATION'] %>
authentication: :plain
Copy link
Member

Choose a reason for hiding this comment

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

@dkastl (CC: @smellman)
Well, after I read the following Redmine Email Configuration again, I prefer to keep this SMTP_AUTHENTICATION configurable.
https://www.redmine.org/projects/redmine/wiki/EmailConfiguration

Is there some reason to drop this configuration ?

[email protected]

# System
RAILS_ENV=production
Copy link
Member

Choose a reason for hiding this comment

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

@dkastl (CC: @smellman @nobnisai)
From yesterday's internal discussion, we agreed not to support other development and test environments, so this RAILS_ENV line will not be necessary. (Because, current code doesn't support configuration.)

Note that plugin's test will not be available if we stick to production environment, so to develop plugins with tests, we need to setup another environment.

# System
RAILS_ENV=production
RAILS_LOG_TO_STDOUT=1
RAILS_MEMORY_LIMIT=1024
Copy link
Member

Choose a reason for hiding this comment

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

puma.rb file's RAILS_MEMORY_LIMIT default value is 2048, so it's better to change 1024 to 2048.
https://github.com/gtt-project/docker-gtt/pull/54/files#diff-24409379bdb75ad446bb4e2c18fd4cced1a263b99a3ff96fc3777d8fd8faeab9

RAILS_ENV=production
RAILS_LOG_TO_STDOUT=1
RAILS_MEMORY_LIMIT=1024
PUMA_WORKERS=3
Copy link
Member

Choose a reason for hiding this comment

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

@dkastl (CC: @smellman, @nobnisai)
This is nice enhancement!

When I set PUMA_WORKERS=0, then it seems not to launch multiple workers and killers.

docker-gtt-redmine-1  | Puma starting in single mode...
docker-gtt-redmine-1  | * Puma version: 5.6.5 (ruby 3.1.2-p20) ("Birdie's Version")
docker-gtt-redmine-1  | *  Min threads: 0
docker-gtt-redmine-1  | *  Max threads: 5
docker-gtt-redmine-1  | *  Environment: production
docker-gtt-redmine-1  | *          PID: 1
docker-gtt-redmine-1  | * Listening on http://0.0.0.0:3000
docker-gtt-redmine-1  | Use Ctrl-C to stop

The following is the default (PUMA_WORKERS=3).

docker-gtt-redmine-1  | [1] Puma starting in cluster mode...
docker-gtt-redmine-1  | [1] * Puma version: 5.6.5 (ruby 3.1.2-p20) ("Birdie's Version")
docker-gtt-redmine-1  | [1] *  Min threads: 0
docker-gtt-redmine-1  | [1] *  Max threads: 5
docker-gtt-redmine-1  | [1] *  Environment: production
docker-gtt-redmine-1  | [1] *   Master PID: 1
docker-gtt-redmine-1  | [1] *      Workers: 3
docker-gtt-redmine-1  | [1] *     Restarts: (✔) hot (✔) phased
docker-gtt-redmine-1  | [1] * Listening on http://0.0.0.0:3000
docker-gtt-redmine-1  | [1] Use Ctrl-C to stop
docker-gtt-redmine-1  | [1] - Worker 0 (PID: 21) booted in 0.0s, phase: 0
docker-gtt-redmine-1  | [1] - Worker 1 (PID: 23) booted in 0.0s, phase: 0
docker-gtt-redmine-1  | [1] - Worker 2 (PID: 25) booted in 0.0s, phase: 0

apt install -y git; \
cd redmine_gtt; \
yarn; \
yarn webpack
Copy link
Member

Choose a reason for hiding this comment

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

This is a slight thing, but in redmine_gtt plugin's README.md, we use npx webpack, but is it better to change yarn webpack in the README.md side ?
https://github.com/gtt-project/redmine_gtt#installation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Redmine 5.0 with simplifying docker settings
2 participants