Without this change, `npm run postinstall` (aka
scripts/copy-node-modules.sh) uses `set -x`, which echo steps to STDERR.
By default, it seems that GitHub Actions doesn't show STDERR.
Having the steps visible in CI was very useful while debugging some
Tutor build improvements, so I figured it would be good to upstream the
change.
This should probably be turned into an ADR at some point, but for
now I've just recreated the page on the public wiki. (The space it
was in had been deleted during the 2U/Axim split.)
Implements the `npm run watch` section of the assets ADR [1], plus some
modifications since I decided to switch from pywatchman to watchdog (see
ADR changes for justification). This will replace `paver watch_assets`
(edx-platform) and `openedx-assets watch-themes` (Tutor).
Specifically, this PR adds three experimental commands:
* `npm run watch-sass` : Watch for Sass changes with watchdog.
* `npm run watch-webpack` : Invoke Webpack-watch for JS changes.
* `npm run watch` : Invoke both `watch-sass` and `watch-webpack` simultaneously.
These commands are only intended to work in development mode. They have
been tested both on bare-metal edx-platform and through `tutor dev run`
on on Linux.
Before removing the "experimental" label, we need to:
* Test through Devstack on Linux.
* Test through Devstack and `tutor dev run` on macOS.
* Test on bare-metal macOS. Might not work, which is OK, but we should
document that.
* Document the commands in edx-platform's README.
* Confirm that this not only works through `tutor dev run`, but also as
a suitable replacement in the `watchthemes` service that Tutor runs
automatically as part of `tutor dev start`. Tweak if necessary.
References:
1. https://github.com/openedx/edx-platform/blob/master/docs/decisions/0017-reimplement-asset-processing.rst
Part of: https://github.com/openedx/edx-platform/issues/31612
This PR implements much of the static assets rework ADR [1], including:
* `npm run build[-dev]`, and its subcommands,
* `npm run webpack[-dev]` and
* `npm run compile-sass[-dev]`.
This is backwards-compatible. `paver update_assets` should not be affected.
The new command warns that it is "experimental" for a few reasons:
* `npm run build` will fail in the webpack phase unless you first
run `xmodule_assets`. This will be changed soon [2].
* We have tested the new build, but not quite so thoroughly that we'd
recommend it as the production default yet. Once the xmodule_assets
work lands, we'll share this on the forums so early adopters can try it
out.
* The commands lack some top-level documentation. Once they stabilize more,
we'll add a section to the README that explains how and when to use `npm run
build` and its subcommands and its env vars.
* `npm run watch` is not yet implemented.
References:
1. https://github.com/openedx/edx-platform/blob/master/docs/decisions/0017-reimplement-asset-processing.rst
2. https://github.com/openedx/edx-platform/pull/32685
Part of: https://github.com/openedx/edx-platform/issues/31604
During the review of ADR 17 [1], Régis pointed out [2] that the shell script
which replaces Paver's `process_npm_assets` could be automatically invoked as
an NPM post-install hook, ensuring that the step is seamlessly executed whenever
`npm install` is run. I had avoided using that suggestion, as I worried that it
would make it harder to move node_modules out of the edx-platform directory in
Tutor's openedx image.
Since then, two things have changed. Firstly, Tutor v16's new persistent mounts
interface [3] has lessened the importance of moving node_modules. Secondly, I
have realized that using a post-install hook would not preclude us from
modifying the underlying script (scripts/copy-node-modules.sh) to look in an
alternative location for node_modules, should that end up being something we
want to do.
This commit modifies the ADR based on those findings, stubs out Paver's
`process_npm_assets`, and adds the suggested post-install hook and replacement
Bash script.
References:
1. https://github.com/openedx/edx-platform/blob/master/docs/decisions/0017-reimplement-asset-processing.rst
2. https://github.com/openedx/edx-platform/pull/31790#discussion_r1122802492
3. https://github.com/overhangio/tutor/blob/master/CHANGELOG.md#v1600-2023-06-14
Part of: https://github.com/openedx/edx-platform/issues/31604
* feat: Add a script to enhance JWKs in preparation for move from pyjwkest
This script accepts a signing JWK (presumably `JWT_PRIVATE_SIGNING_JWK`)
and ensures that it has all of the precomputed private numbers that are
required for top performance. This is necessary before moving away from
pyjwkest to PyJWT for signing JWTs. See issue
<https://github.com/openedx/edx-drf-extensions/issues/290>. (Alternatively,
one could remove the p, q, dp, dq, and qi params, but there is an unknown
performance cost to doing so as we are not currently caching these keys,
and the precompution happens on every load due to the way pyjwkest's API
works.)
* fixup! Upgrade devstack at the same time
There was a `requirements/pip.txt` with old versions, and a newer
`requirements/edx/pip.txt` managed via a `pip.in` file. The old one was
used in most places, but came out of sync with pip-tools.txt, which was
managed properly. Eventually this caused a `pip check` failure due to the
mismatch.
This should resolve at least part of https://github.com/edx/edx-arch-experiments/issues/267
This PR moves pip.in and pip-tools.in and their corresponding pin files
up to the `requirements/` dir, since they should be shared between the edx
and sandbox environments. This also has the effect of upgrading pip to
match the version in the file we've been uselessly upgrading.
Other improvements:
- Remove `-q` option from pip and pip-sync calls, as it was hiding some
debugging information that would have resolved this sooner.
- Depend on `pre-requirements` from `compile-requirements`, rather than
from `upgrade`. (The base target is the one that actually needs it.)
This also lets us remove the explicit `pip install pip-tools` line.
- Install the recompiled pip and pip-tools files right away, not after the
loop. When we upgrade pip-tools, we want to use the upgraded version,
not the previous version. This requires moving the pip-tools.txt
recompilation outside of the loop and into its own explicit line.
- Don't upgrade pip if we're not running `make upgrade` (respect the
compile options).
- Remove apparently-unneeded `--no-emit-trusted-host --no-emit-index-url`
options (we don't pass trusted-host or index-url options).
build: run ShellCheck
Adds a ShellCheck check to edx-platform PRs and master,
using the shared workflow & template from the .github repo.
This will become a "required" check once it passes for 2 straight weeks on master.
Brings all existing shell scripts into compliance with ShellCheck.
These scripts were written to replace some of the
Ansible provisioning steps in Devstack,
before tCRIL's focus turned to Tutor.
These scripts never ended up being used by Devstack, but
Devstack's maintainers did end up replacing
the Ansible provisioning steps with some direct
Bash commands in Devstack's provision-lms.sh:
23e16c1dda/provision-lms.sh (L51-L68)
Tutor has its own user & course provisioning scripts,
written in Sh using Tutor's jobs framework.
Thus, these scripts are no longer needed and do
not seem to be in use.
The file was introduced as part of the "decentralized devstack" spike.
We wanted a working Dockerfile in the edx-platform root,
and we wanted to speed it up by not relying on Paver.
edx-platform now has a working Dockerfile, and it does not use this script.
The paver-free-assets initiative is being continued here:
https://github.com/openedx/edx-platform/issues/31798
It will result in a new, prod-ready asset building script.
These files were used to run tests on
build.testeng.edx.org, a Jenkins instance.
All tests have since been moved over to
GitHub Actions. The build.testeng.edx.org
instance is decomissioned.
Removed files include:
- scripts/jenkins-*.sh
- scripts/Jenkinsfiles/*
- scripts/xdist/*
Note on xdist scripts: pytest-xdist in general is still
useful for local test runs in order to take advantage
of all available CPU cores, but the scripts here were
specifically for running tests on multiple remote machines,
particularly via Jenkins.
We have a need to lock the version of Django for production and tests, but
also to test on newer versions of Django so that we can get the repo ready
for long-term-support releases.
We've been doing that by extracting the `django==x.y.z` from the
pip-compiled files and moving it to a django.txt that is then co-installed
but can be overridden during tests. The problem is that this can result
in broken packages.
The approach here is to have `make test-requirements` continue to
ensure a consistent set of packages, and then install a different
Django on top of that in the CI script -- and call `pip check` to make
sure that combination isn't broken.
Adding Django 4.0 to the unit-tests.yml matrix will now correctly
result in this error and a failing job:
`django-splash 1.2.1 has requirement Django<4.0, but you have django 4.0.8.`
The other half of this is to change other CI runners to remove their
ability to control the Django version, since it's complicated to make
this work, and we probably only need it in unit-tests.yml. Convert them
to just use `make test-requirements`.
Also:
- Simplify handling of `pip --src` by setting `PIP_SRC` (rather than our
own `PIP_SRC_DIR`, which pip ignores because `--src-dir` isn't an option
that it knows). This is needed to allow `make test-requirements` to do
the pip calls. An alternative would be to set a pip-options env var for
the make target to use, but `PIP_SRC` already exists.
- Remove outdated modifications to common_constraints
- Add comment explaining why pylint tests need dev-requirements
