Try to build static files on github actions #35427
Conversation
…i/commcare-hq into ap/build-static-on-actions
AmitPhulera
changed the title
| ./manage.py resource_static | ||
| echo "Finished resource_static." |
There was a problem hiding this comment.
super nitpicky: Logging after seems like it mainly adds noise to the logs since there is immediately another log message before the next task starts. Would it make sense to remove the echo "Finished ..." lines?
| return parts[-1] | ||
|
|
||
| # Default to last part of path | ||
| return parts[-1] |
There was a problem hiding this comment.
Given that this is the default, the previous two conditions ('contrib' in parts and 'apps' in parts) that also return parts[-1] seem redundant. Would it make sense to remove them?
| return parts[-1] | ||
|
|
||
| # Get list of installed app paths | ||
| installed_apps = {get_app_name(app.name): app.path for app in apps.get_app_configs()} |
There was a problem hiding this comment.
I think it is possible for two apps to have the same result of get_app_name(app.name), although we do not appear to have any at this time. However, this expression would silently omit one of them if that ever happened. For example, if we created corehq.apps.admin it would be a duplicate of django.contrib.admin.
| installed_apps = {get_app_name(app.name): app.path for app in apps.get_app_configs()} | |
| installed_apps = {get_app_name(app.name): app.path for app in apps.get_app_configs()} | |
| from collections import Counter | |
| names = Counter([get_app_name(app.name) for app in apps.get_app_configs()]) | |
| assert names.most_common(1)[0][1] == 1, "found duplicate app names: " \ | |
| + ', '.join(n for n in names if names[n] > 1) |
|
|
||
| def should_copy_directory(dir_name): | ||
| # Skip node_modules | ||
| if 'node_modules' in dir_name: |
There was a problem hiding this comment.
Would == make more sense here? I don't expect it to happen, but imagine if we had an app named anode_modules (it would be excluded). But see the suggestion below before doing that.
| # Skip node_modules | ||
| if 'node_modules' in dir_name: | ||
| return False | ||
|
|
||
| # Check if directory is an installed app | ||
| if dir_name in installed_apps: | ||
| return True | ||
|
|
||
| # Check if directory is in additional_dirs | ||
| if dir_name in additional_dirs: | ||
| return True | ||
|
|
||
| return False |
There was a problem hiding this comment.
| # Skip node_modules | |
| if 'node_modules' in dir_name: | |
| return False | |
| # Check if directory is an installed app | |
| if dir_name in installed_apps: | |
| return True | |
| # Check if directory is in additional_dirs | |
| if dir_name in additional_dirs: | |
| return True | |
| return False | |
| return dir_name in installed_apps or dir_name in additional_dirs |
|
|
||
| if os.path.isdir(src_path) and should_copy_directory(item): | ||
| self.stdout.write(f"Copying {item}...") | ||
| copy_directory(src_path, dst_path) |
There was a problem hiding this comment.
Can copy_directory be replaced with shutil.copytree?
| copy_directory(src_path, dst_path) | |
| shutil.copytree(src_path, dst_path, dirs_exist_ok=True) |
Could also pass ignore=ignore_patterns('node_modules') if these directories may contain node_modules that should be excluded. Is that necessary?
| # Split on dots and get the last part | ||
| parts = app_path.split('.') | ||
|
|
||
| # Handle special cases like 'django.contrib.admin' -> 'admin' | ||
| if 'contrib' in parts: | ||
| return parts[-1] | ||
|
|
||
| # Handle cases like 'corehq.apps.users' -> 'users' | ||
| if 'apps' in parts: | ||
| return parts[-1] | ||
|
|
||
| # Default to last part of path | ||
| return parts[-1] |
There was a problem hiding this comment.
Given that this is the default, the previous two conditions ('contrib' in parts and 'apps' in parts) that also return parts[-1] seem redundant. Would it make sense to remove them?
| # Split on dots and get the last part | |
| parts = app_path.split('.') | |
| # Handle special cases like 'django.contrib.admin' -> 'admin' | |
| if 'contrib' in parts: | |
| return parts[-1] | |
| # Handle cases like 'corehq.apps.users' -> 'users' | |
| if 'apps' in parts: | |
| return parts[-1] | |
| # Default to last part of path | |
| return parts[-1] | |
| return app_path.rsplit('.', 1)[-1] |
Product Description
This PR builds the static files and upload them as github artifacts. As of now it will only build staticfiles when anything is pushed on
autostagingbranch.It does not affect anything on production. Just introduces one management command that will run on actions.
Safety Assurance
Pretty safe, have a very limited scope and that is also outside production for now.
Safety story
Automated test coverage
N/A
QA Plan
N/A
Rollback instructions
Labels & Review