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

Update dockerfile to speed up builds #741

Merged
merged 3 commits into from
Nov 13, 2023
Merged

Conversation

jarosenb
Copy link
Collaborator

@jarosenb jarosenb commented Nov 2, 2023

Overview

Update the Dockerfile to remove Node deprecation warnings in CI and streamline Python dependency installation

Related

Changes

  • Use a separate Node build step to handle style building
  • Update Poetry config to use the container's system Python
  • expose the Poetry binary when Core-CMS is used as a base image.

@jarosenb jarosenb changed the title update dockerfile to speed up builds Update dockerfile to speed up builds Nov 2, 2023
Copy link
Member

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

Looks good.

(As a separate task) Would it be inappropriate to run python manage.py collectstatic via the Dockerfile? I think this might save devs1 and CI that step in client apps.

Answer
[It] makes sense for collectstatic to happen downstream. Static files need to live in a volume that's shared with the Nginx container. IDK if that setup works if they're baked into the container. Also anything that uses core-cms as a base image will need to run collectstatic again anyway. e.g. tup needs to do it to pick up tup-ui.


Also, I should test this PR thoroughly.

  • Core-CMS local
  • Core-CMS remote
  • Core-CMS-Resources local
  • Core-CMS-Resources remote
  • Core-CMS-Custom local
  • Core-CMS-Custom remote
  • tup-ui local (I have a clone)
  • tup-ui remote

Footnotes

  1. The collectstatic might be unnecessary for devs, if they have DEBUG set to True, because urls.py#L82-L83 (since 1c326b9).2

  2. 2023-01-16: v4.4.1 README suggests devs might still need to run collectstatic.

Copy link
Collaborator

@taoteg taoteg left a comment

Choose a reason for hiding this comment

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

LGTM! Nice update.

Copy link
Member

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

I think I understand this well now. Based on my understanding, I tested enough.

@wesleyboar wesleyboar merged commit 398370b into main Nov 13, 2023
@wesleyboar wesleyboar deleted the task/WI-63--cms-build-speed branch November 13, 2023 20:12
wesleyboar added a commit that referenced this pull request Nov 16, 2023
Clean up neglected by #741.
@wesleyboar wesleyboar mentioned this pull request Nov 28, 2023
wesleyboar added a commit to TACC/Core-CMS-Resources that referenced this pull request Nov 28, 2023
wesleyboar added a commit that referenced this pull request Nov 28, 2023
wesleyboar added a commit to TACC/Core-CMS-Resources that referenced this pull request Nov 28, 2023
* docs: update docs since TACC/Core-CMS#741

* docs: restore debug-project.md
wesleyboar added a commit that referenced this pull request Nov 28, 2023
* docs: update docs since #741

* docs: restore taccsite_custom/docs/debug-project

* chore: taccsite_custom checkout main (PR merged)
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.

4 participants