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

[bitnami/discourse] Use unicorn instead of passenger #2355

Closed
wants to merge 1 commit into from

Conversation

wzrdtales
Copy link

Original https://github.com/bitnami/bitnami-docker-discourse/pull/234

discourse lately removed the setting from the UI, this image doesn't really work properly without disabling this option currently.

Signed-off-by: Tobias Gurtzick [email protected]


right now requires to set serve_static_assets = true


important info from previous ticket:

the docker-compose seems to be outdated and deviates from the helm chart.
The helm chart sets already

  - DISCOURSE_PORT_NUMBER=8080
  - DISCOURSE_EXTERNAL_HTTP_PORT_NUMBER=80

These two variables, and adding those to the docker-compose lets it run just fine.

@wzrdtales
Copy link
Author

/cc @carrodher

@bitnami-bot bitnami-bot requested a review from rafariossaa August 8, 2022 19:33
@bitnami-bot bitnami-bot added the triage Triage is needed label Aug 8, 2022
@wzrdtales
Copy link
Author

/cc @migruiz4 @belmeopmenieuwesim

@recena recena changed the title Use unicorn instead of passenger [bitnami/discourse] Use unicorn instead of passenger Aug 9, 2022
@bitnami-bot bitnami-bot added in-progress and removed triage Triage is needed labels Aug 11, 2022
@bitnami-bot bitnami-bot removed the request for review from rafariossaa August 11, 2022 07:10
@bitnami-bot bitnami-bot requested a review from gongomgra August 11, 2022 07:10
@gongomgra gongomgra requested review from migruiz4 and removed request for gongomgra August 16, 2022 10:01
@gongomgra
Copy link
Contributor

@migruiz4, as you have more context than me with this PR and it looks like you have already started checking the changes in the associated issue, I'm handling it over to you so you can take the best decision.

@gongomgra gongomgra removed their assignment Aug 16, 2022
Copy link
Member

@migruiz4 migruiz4 left a comment

Choose a reason for hiding this comment

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

Hi @wzrdtales,

Please take a look at my comments below.

I have suggested several changes, including the removal of runit as it does not seem necessary to run unicorn.

Comment on lines +96 to +97
DISCOURSE_SERVE_STATIC_ASSETS="${DISCOURSE_SERVE_STATIC_ASSETS:-true}"
export DISCOURSE_SERVE_STATIC_ASSETS="${DISCOURSE_SERVE_STATIC_ASSETS:-true}"
Copy link
Member

Choose a reason for hiding this comment

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

Please move these lines under the # Discourse configuration comment

@@ -17,7 +17,7 @@ RUN . /opt/bitnami/scripts/libcomponent.sh && component_unpack "node" "14.20.0-1
RUN . /opt/bitnami/scripts/libcomponent.sh && component_unpack "git" "2.37.1-1" --checksum 39ea3040baa552b4760c1100a3713f86493620c0a74121c1ceba50fe17341878
RUN . /opt/bitnami/scripts/libcomponent.sh && component_unpack "brotli" "1.0.9-151" --checksum ba4c4d26d232f8d91d1143078a633ba7c5ae33f5c4182684400ffb7bbc0ed262
RUN . /opt/bitnami/scripts/libcomponent.sh && component_unpack "discourse" "2.8.7-1" --checksum bd120eab1a7403e691f608f57b574a9782391fe1c4a46ce40752cfa560f922c2
RUN apt-get update && apt-get upgrade -y && \
RUN apt-get update && apt-get upgrade -y && apt-get install sudo runit -y && \
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer not installing runit in a container unless it is strictly necessary.

I have performed some tests using the changes suggested in this review and I was able to use unicorn without runit.

Copy link
Author

Choose a reason for hiding this comment

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

if you change something that supposedly is working, then it would be nice if you just attach it. I don't have time to test a foreign theory. When we tested it didn't work without runit to run as non-root

"-p" "$DISCOURSE_PORT_NUMBER"
"--spawn-method" "$DISCOURSE_PASSENGER_SPAWN_METHOD"
"-c" "config/unicorn.conf.rb"
)

# Append extra flags specified via environment variables
Copy link
Member

Choose a reason for hiding this comment

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

Please rename PASSENGER env variables.

@@ -92,6 +93,8 @@ export DISCOURSE_CONF_FILE="${DISCOURSE_BASE_DIR}/config/discourse.conf"
export PATH="${BITNAMI_ROOT_DIR}/common/bin:${BITNAMI_ROOT_DIR}/brotli/bin:${BITNAMI_ROOT_DIR}/git/bin:${PATH}"

# Discourse persistence configuration
DISCOURSE_SERVE_STATIC_ASSETS="${DISCOURSE_SERVE_STATIC_ASSETS:-true}"
export DISCOURSE_SERVE_STATIC_ASSETS="${DISCOURSE_SERVE_STATIC_ASSETS:-true}"
export DISCOURSE_VOLUME_DIR="${BITNAMI_VOLUME_DIR}/discourse"
export DISCOURSE_DATA_TO_PERSIST="${DISCOURSE_DATA_TO_PERSIST:-plugins public/backups public/uploads}"

Copy link
Member

Choose a reason for hiding this comment

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

Unicorn will fail to start if we use the current default value for DISCOURSE_PORT_NUMBER, please change the value from 3000 to 8080.

export DISCOURSE_PORT_NUMBER="${DISCOURSE_PORT_NUMBER:-8080}"

In addition, please replace the ports value at docker-compose.yaml.

"bundle" "exec" "passenger" "start"
"--user" "$DISCOURSE_DAEMON_USER"
"-e" "$DISCOURSE_ENV"
chpst -u "$DISCOURSE_DAEMON_USER" -U "$DISCOURSE_DAEMON_USER" "bundle" "exec" "config/unicorn_launcher"
Copy link
Member

Choose a reason for hiding this comment

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

No need to use chpst if runit won't be used

Suggested change
chpst -u "$DISCOURSE_DAEMON_USER" -U "$DISCOURSE_DAEMON_USER" "bundle" "exec" "config/unicorn_launcher"
"bundle" "exec" "config/unicorn_launcher"

@@ -33,4 +32,4 @@ if [[ -n "$DISCOURSE_PASSENGER_EXTRA_FLAGS" ]]; then
fi

info "** Starting Discourse **"
exec "${cmd[@]}" "$@"
USER=$DISCOURSE_DAEMON_USER exec "${cmd[@]}" "$@"
Copy link
Member

Choose a reason for hiding this comment

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

To avoid running the process as root, use the following instead:

Suggested change
USER=$DISCOURSE_DAEMON_USER exec "${cmd[@]}" "$@"
if am_i_root; then
exec gosu "$DISCOURSE_DAEMON_USER" "${cmd[@]}"
else
exec "${cmd[@]}"
fi

@github-actions
Copy link

This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution.

@github-actions
Copy link

Due to the lack of activity in the last 5 days since it was marked as "stale", we proceed to close this Pull Request. Do not hesitate to reopen it later if necessary.

@github-actions github-actions bot closed this Sep 16, 2022
@javsalgar javsalgar reopened this Oct 11, 2023
@github-actions github-actions bot added in-progress and removed triage Triage is needed labels Oct 11, 2023
@bitnami-bot bitnami-bot removed the request for review from migruiz4 October 11, 2023 10:04
@github-actions github-actions bot added triage Triage is needed and removed in-progress labels Oct 11, 2023
@github-actions github-actions bot removed the stale 15 days without activity label Oct 12, 2023
@belmeopmenieuwesim
Copy link

this stuff is unmaintained. If I were you I would look elsewhere.

@wzrdtales
Copy link
Author

yeah, we're maintaining it until today ourselves in a forked image... .

Copy link

github-actions bot commented Nov 1, 2023

This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution.

@github-actions github-actions bot added the stale 15 days without activity label Nov 1, 2023
@CeliaGMqrz CeliaGMqrz removed the stale 15 days without activity label Nov 2, 2023
@jacobowitz
Copy link

@migruiz4
Can I assume that you will pick up this PR eventually? I need this PR merged as well. If nobody is picking this one up, I'd try, but I am lacking a lot of context here so I would rather not do it.

@migruiz4
Copy link
Member

Hi @jacobowitz,

We have an internal task to study if migrating bitnami/unicorn from passenger to unicorn is viable or not, but as we have to deal with other priorities it is currently in our Backlog.

As I mentioned in my review, the usage of runit in our images is a strong no from my side.

Copy link

github-actions bot commented Dec 3, 2023

This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution.

@github-actions github-actions bot added the stale 15 days without activity label Dec 3, 2023
Copy link

github-actions bot commented Dec 9, 2023

Due to the lack of activity in the last 5 days since it was marked as "stale", we proceed to close this Pull Request. Do not hesitate to reopen it later if necessary.

@github-actions github-actions bot added the solved label Dec 9, 2023
@bitnami-bot bitnami-bot added stale 15 days without activity and removed stale 15 days without activity labels Dec 9, 2023
@bitnami-bot bitnami-bot closed this Dec 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discourse solved stale 15 days without activity triage Triage is needed verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.