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

Add Puma via govuk_app_config #876

Merged
merged 1 commit into from
Oct 27, 2021
Merged

Add Puma via govuk_app_config #876

merged 1 commit into from
Oct 27, 2021

Conversation

karlbaker02
Copy link
Contributor

@karlbaker02 karlbaker02 commented Oct 19, 2021

This PR adds Puma via govuk_app_config. Puma was previously installed indirectly through the dependency on govuk_test as part of the dev and test groups; given that we want to run Puma in production, we are instead configuring it via govuk_app_config.

This PR is reliant on changes to govuk_app_config

Trello card

@kevindew
Copy link
Member

This seems a reasonable quick fix if that's what needed, however the more conventional approach would be to add puma to govuk_app_config (and include any common GOV.UK configuration there) - this is how unicorn is installed for all the apps. Doing it via govuk_app_config would also mean that you apply it to govuk_app_config once and then it'd likely end up being merged into all the apps before you need to do any other manual fixes.

@karlbaker02
Copy link
Contributor Author

This seems a reasonable quick fix if that's what needed, however the more conventional approach would be to add puma to govuk_app_config (and include any common GOV.UK configuration there) - this is how unicorn is installed for all the apps. Doing it via govuk_app_config would also mean that you apply it to govuk_app_config once and then it'd likely end up being merged into all the apps before you need to do any other manual fixes.

We'll revisit whether to do this now or later. In terms of app config I agree too - currently some apps already have a Puma config and some don't, not to mention that the configs are inconsistent; it'd be good to have a single standard and canonical source of truth, which is what we're going to do.

@karlbaker02
Copy link
Contributor Author

To expand on this, I don't think we want to make the switch to use Puma yet in the same way that Unicorn is defined (by using puma.rb to call for example a new GovukPuma defined in govuk_app_config), because we don't want to switch over apps in the existing stack to use Puma. The idea is that as part of the replatforming work, we will switch over to Puma when the apps are run on K8s, so to facilitate this we're changing the app Dockerfiles to explicitly state this as the CMD (equally it can be defined in K8s deployments).

Once we're running on K8s and the old stack has been deprecated, it should be fairly simple to take these configs and standardise them by setting them in govuk_app_config.

@sengi do you agree with this assessment?

@kevindew
Copy link
Member

To expand on this, I don't think we want to make the switch to use Puma yet in the same way that Unicorn is defined (by using puma.rb to call for example a new GovukPuma defined in govuk_app_config), because we don't want to switch over apps in the existing stack to use Puma. The idea is that as part of the replatforming work, we will switch over to Puma when the apps are run on K8s, so to facilitate this we're changing the app Dockerfiles to explicitly state this as the CMD (equally it can be defined in K8s deployments).

Once we're running on K8s and the old stack has been deprecated, it should be fairly simple to take these configs and standardise them by setting them in govuk_app_config.

@sengi do you agree with this assessment?

One thing to be aware of is that setting anything up in govuk_app_config won't mean anything changes automatically for our apps. They're start a specific unicorn process, so won't switch over. You won't need to wait for the old stack to be deprecated before adding things there - if anything, it sounds like a lot of admin to update every app first and then move it to govuk_app_config - and it's pretty low cost as nearly all the apps already have puma as an indirect dependency through govuk_test

@karlbaker02 karlbaker02 changed the title Add Puma as direct dependency Add Puma via govuk_app_config Oct 20, 2021
This commit adds `Puma` via govuk_app_config. Puma was previously installed indirectly through the dependency on `govuk_test` as part of the `dev` and `test` groups; given that we want to run Puma in production, we are instead configuring it via govuk_app_config.

Co-authored-by: Chris Banks <chris.banks@digital.cabinet-office.gov.uk>
@karlbaker02 karlbaker02 merged commit c0b906f into main Oct 27, 2021
@karlbaker02 karlbaker02 deleted the add-puma branch October 27, 2021 09:09
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