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

feat: install npm private requirements during frontend build #1

Merged
merged 2 commits into from
May 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions tubular/scripts/frontend_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ def frontend_build(common_config_file, env_config_file, app_name, version_file):
builder = FrontendBuilder(common_config_file, env_config_file, app_name, version_file)
builder.install_requirements()
app_config = builder.get_app_config()
builder.copy_js_config_file_to_app_root(app_config, app_name)
env_vars = [
f"{k}={ensure_wrapped_in_quotes(v)}"
for k, v in app_config.items()
Expand Down
43 changes: 43 additions & 0 deletions tubular/scripts/frontend_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import io
import json
import os
import shutil
import subprocess
import sys

Expand Down Expand Up @@ -62,6 +63,7 @@ def install_requirements(self):
self.FAIL(1, 'Could not run `npm install` for app {}.'.format(self.app_name))

self.install_requirements_npm_aliases()
self.install_requirements_npm_private()

def install_requirements_npm_aliases(self):
""" Install npm alias requirements for app to build """
Expand All @@ -86,6 +88,22 @@ def install_requirements_npm_aliases(self):
aliased_installs, self.app_name
))

def install_requirements_npm_private(self):
""" Install npm private requirements for app to build """
npm_private = self.get_npm_private_config()
if npm_private:
install_list = ' '.join(npm_private)
install_private_proc = subprocess.Popen(
[f'npm install {install_list}'],
cwd=self.app_name,
shell=True
)
install_private_proc_return_code = install_private_proc.wait()
if install_private_proc_return_code != 0:
self.FAIL(1, 'Could not run `npm install {}` for app {}.'.format(
install_list, self.app_name
))

def get_app_config(self):
""" Combines the common and environment configs APP_CONFIG data """
app_config = self.common_cfg.get('APP_CONFIG', {})
Expand All @@ -102,6 +120,14 @@ def get_npm_aliases_config(self):
self.LOG('No npm package aliases defined in config.')
return npm_aliases_config

def get_npm_private_config(self):
Copy link

@leangseu-edx leangseu-edx May 3, 2024

Choose a reason for hiding this comment

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

this can be achieved with NPM_ALIASES. I don't think we need to add another implementation.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, the difference between NPM_ALIASES and NPM_PRIVATE is that dependencies in NPM_PRIVATE are not aliases. If we relied on NPM_ALIASES, the syntax might be somewhat unnecessarily verbose/confusing to define an NPM alias for the same package name it already has.

NPM_ALIASES:
  - "@datadog/browser-rum": "npm:@datadog/browser-rum@2"

versus:

NPM_PRIVATE:
  - "@datadog/browser-rum@2"

""" Combines the common and environment configs NPM_PRIVATE packages """
npm_private_config = self.common_cfg.get('NPM_PRIVATE', [])
npm_private_config.extend(self.env_cfg.get('NPM_PRIVATE', []))
if not npm_private_config:
self.LOG('No npm private packages defined in config.')
return list(set(npm_private_config))

def build_app(self, env_vars, fail_msg):
""" Builds the app with environment variable."""
proc = subprocess.Popen(
Expand All @@ -127,6 +153,23 @@ def create_version_file(self):
except IOError:
self.FAIL(1, 'Could not write to version file for app {}.'.format(self.app_name))

def copy_js_config_file_to_app_root(self, app_config, app_name):
""" Creates a copy of env.config.js(x) file to the app root """
source = app_config.get('JS_CONFIG_FILEPATH')

# Skip if JS_CONFIG_FILEPATH not defined
if not source:
return

filename = app_config.get('LOCAL_JS_CONFIG_FILENAME', "env.config.js")
Copy link
Member

Choose a reason for hiding this comment

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

nit: is it worth having one extra check on line 161 if app_config.get('LOCAL_JS_CONFIG_FILENAME') does not exist? Though it should exist since you're defining it in a common location applicable to all MFEs, an MFE could override it to be an empty config settings.

destination = f"{app_name}/{filename}"
try:
shutil.copyfile(source, destination)
except FileNotFoundError:
self.FAIL(1, f"Could not find '{source}' for copying for app {app_name}.")
except OSError:
self.FAIL(1, f"Could not copy '{source}' to '{destination}', due to destination not writable.")


class FrontendDeployer:
""" Utility class for deploying frontends. """
Expand Down
5 changes: 5 additions & 0 deletions tubular/tests/example-frontend-config/app.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,8 @@ APP_CONFIG:
# Check None. Should all yield `key='None'`.
NONE: !!null
NONE_WITH_QUOTES: 'None' # `None` instead of `null` because Python loads this file.

# This can be overridden for any MFE
LOCAL_JS_CONFIG_FILENAME: "env.config.jsx"

JS_CONFIG_FILEPATH: "dummy/file/path/env.stage.config.jsx"
3 changes: 3 additions & 0 deletions tubular/tests/example-frontend-config/common.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,6 @@ APP_CONFIG:

# Make sure common values can be overriden by app config.
COMMON_OVERRIDE_ME: initial_value

# Local Config File Name
LOCAL_JS_CONFIG_FILENAME: "env.config.js"
8 changes: 7 additions & 1 deletion tubular/tests/test_frontend_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@ class TestFrontendBuildConfigHandling(TestCase):
Cursory tests for frontend config parsing + marshalling.
"""

@mock.patch('tubular.scripts.frontend_utils.shutil.copyfile')
@mock.patch.object(FrontendBuilder, 'create_version_file')
@mock.patch.object(FrontendBuilder, 'build_app')
@mock.patch.object(FrontendBuilder, 'install_requirements')
def test_frontend_build_config_handling(
self, mock_install, mock_build, mock_create_version
self, mock_install, mock_build, mock_create_version, mock_shutil_copyfile
):
exit_code = None
try:
Expand All @@ -46,6 +47,7 @@ def test_frontend_build_config_handling(
assert mock_build.call_args[0][0] == [
"COMMON_VAR='common_value'",
"COMMON_OVERRIDE_ME='overriden_value'",
"LOCAL_JS_CONFIG_FILENAME='env.config.jsx'",
"VAR_WITH_SINGLE_QUOTES='The // value!'",
"VAR_WITH_DOUBLE_QUOTES='The // value!'",
"VAR_WITH_SINGLE_THEN_DOUBLE_QUOTES='The // value!'",
Expand All @@ -62,5 +64,9 @@ def test_frontend_build_config_handling(
"BOOL_FALSE_WITH_QUOTES='False'",
"NONE='None'",
"NONE_WITH_QUOTES='None'",
"JS_CONFIG_FILEPATH='dummy/file/path/env.stage.config.jsx'",
]
assert mock_create_version.call_count == 1
assert mock_shutil_copyfile.call_count == 1
# Verify that source is correct and destination is rightly formatted
mock_shutil_copyfile.assert_called_with('dummy/file/path/env.stage.config.jsx', 'coolfrontend/env.config.jsx')
Loading