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.

Sign up for GitHub

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

Jump to bottom

Added webpack to the HQ deploy process #6284

Merged
merged 5 commits into from
May 10, 2024
Merged

Added webpack to the HQ deploy process #6284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9afbf3a
Added webpack to the HQ deploy process
mjriley May 3, 2024
4b96ae0
Switch webpack to only proxy servers
mjriley May 3, 2024
27adc15
Added webworkers to webpack
mjriley May 3, 2024
2adab7a
Added public variable to control webpack execution
mjriley May 7, 2024
4a9484e
Switched to including the role for cleaner log output
mjriley May 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions environments/staging/public.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,3 +238,5 @@ formplayer_java_version: "{{ java_17_bin_path }}/java"
maintenance_start_time: "06:00"

maintenance_end_time: "08:00"

use_webpack: true
10 changes: 10 additions & 0 deletions src/commcare_cloud/ansible/deploy_hq.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,16 @@
vars:
if_not_done: yarn_install

- name: Run webpack
hosts: [webworkers, proxy]
any_errors_fatal: true
tasks:
- include_role: {name: deploy_hq}
vars:
if_not_done: run_webpack
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of adding the tasks in this file to staticfiles_collect.yml file? It would reduce deploy overhead to group them together since they are applied to the same set of hosts. This is doing the same kind of thing as other staticfiles collection tasks, so I think it makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the primary motivation to reduce overhead or to create natural groups? From running this deploy process on staging, I don't think overhead should be a concern -- the webworkers task executed very quickly. The bottleneck is the compilejs18n command from staticfiles_collect.yml. If the motivation is instead to create natural groups, I'd argue that this grouping has the potential to create a lot of confusion. Webpack doesn't collect any static files. Instead, it is generally known as the step to compress/minify, and our dedicated "Compress static files" task lives outside of "Collect static files". I also wouldn't recommend combining tasks just because they use the same hosts. I suppressed a warning by including webworkers here, but in my head, this should really only be deployed to proxy servers.

If we wanted to split off a playbook dedicated to javascript tasks, that would make sense, but I think dedicated tasks for webpack make sense

Copy link
Contributor

Choose a reason for hiding this comment

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

The overhead I'm speaking of is SSH connections made by Ansible, not the time it takes to actually run webpack.

staticfiles_collect.yml is where we do other js-related build steps (example: build_requirejs), so I think it makes sense to put webpack in there too since that is the same type of task.

Copy link
Contributor Author

@mjriley mjriley May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I've updated the associated ticket with logs from 2 runs to compare what changes when webpack is run from the deploy_hq task vs from within staticfiles_collect. I don't believe there is any difference with respect to SSH connections. From what I can tell, ansible will create connections for every task, and it is thus advised to configure SSH to re-use connections.

I continue to believe that moving webpack to staticfiles_collect would be confusing. Collecting into static is django terminology. I think it's unintuitive that we're building requireJS in this playbook, and I don't think we should double down here. Webpack deserves its own task file because it is capable of much more than collecting, and those responsibilities may not align with the same hosts that collecting does. In addition, it is essentially in feature-flag status at the moment. Including it as I'm doing minimizes log output for runs that do not use webpack.

Copy link
Contributor

@millerdev millerdev May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like there would be two extra SSH connections (Check if done: run_webpack and the corresponding Completed: run_webpack) when run_webpack.yml is included via a separate block in this file. Admittedly not much overhead, but it's something.

For me putting it together with other tasks doing similar things (currently in staticfiles_collect.yml) is the more important point since that's where people who are familiar with the existing structure would go to look for that type of task. I could see an argument for renaming staticfiles_collect.yml to something more generic that includes JS bundling. Adding another top-level block in this file seems overkill for a feature flagged experiment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Noting that build_requirejs is the last task in staticfiles_collect.yml, I think because it depends collectstatic. Is there any chance (now or in the future) that webpack could also depend on collectstatic being run first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked at this, and I'm not entirely sure why requireJS runs after collectstatic. That said, webpack is creating artifacts that django will later serve, so those artifacts should be created before static assets are collected. I previously modified the webpack configuration such that webpack is able to follow django app paths, thus it looks for the location of files before collecting them into a single static folder

when: use_webpack | default(false)


- name: Collect static files
hosts: [webworkers, proxy]
any_errors_fatal: true
Expand Down
10 changes: 10 additions & 0 deletions src/commcare_cloud/ansible/roles/deploy_hq/tasks/run_webpack.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
- name: Check for webpack configuration
stat:
path: "{{ code_source }}/webpack.config.js"
register: config_file

- name: Run webpack
command:
cmd: yarn run build:react
chdir: '{{ code_source }}'
when: config_file.stat.exists
Loading