Upgrade authlib - #329
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe PR declares Node.js 18 and Python 3.10, changes dependency initialisation and pins, introduces a multi-stage frontend asset build, redirects copied assets into ChangesFrontend asset build transition
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DockerAssetsStage
participant PackageScripts
participant ApplicationStatic
participant PythonRuntimeImage
DockerAssetsStage->>PackageScripts: run npm run postinstall
PackageScripts->>ApplicationStatic: copy and build frontend assets
DockerAssetsStage->>PythonRuntimeImage: copy application/static
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Dockerfile`:
- Around line 38-42: Reorder the Dockerfile dependency layers so the
requirements files are copied and both pip install commands run before the broad
COPY . . step. Keep the static assets copy and application source copy after
dependency installation, preserving the existing requirements paths and install
behavior while allowing source changes to reuse the dependency cache.
In `@package.json`:
- Around line 18-20: Update the copy:moj-frontend-css script to create
application/static/stylesheets/vendor before running rsync, ensuring the CSS
copy succeeds on a clean build. Keep the existing CSS source and destination
behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8840accf-2281-4ed4-8fe2-f771ac9ebdbe
⛔ Files ignored due to path filters (39)
application/static/govuk/assets/fonts/bold-affa96571d-v2.woffis excluded by!**/*.woffapplication/static/govuk/assets/fonts/bold-b542beb274-v2.woff2is excluded by!**/*.woff2application/static/govuk/assets/fonts/light-94a07e06a1-v2.woff2is excluded by!**/*.woff2application/static/govuk/assets/fonts/light-f591b13f7d-v2.woffis excluded by!**/*.woffapplication/static/govuk/assets/images/favicon.icois excluded by!**/*.icoapplication/static/govuk/assets/images/favicon.svgis excluded by!**/*.svgapplication/static/govuk/assets/images/govuk-crest.svgis excluded by!**/*.svgapplication/static/govuk/assets/images/govuk-icon-180.pngis excluded by!**/*.pngapplication/static/govuk/assets/images/govuk-icon-192.pngis excluded by!**/*.pngapplication/static/govuk/assets/images/govuk-icon-512.pngis excluded by!**/*.pngapplication/static/govuk/assets/images/govuk-icon-mask.svgis excluded by!**/*.svgapplication/static/govuk/assets/images/govuk-opengraph-image.pngis excluded by!**/*.pngapplication/static/govuk/assets/rebrand/images/favicon.icois excluded by!**/*.icoapplication/static/govuk/assets/rebrand/images/favicon.svgis excluded by!**/*.svgapplication/static/govuk/assets/rebrand/images/govuk-crest.svgis excluded by!**/*.svgapplication/static/govuk/assets/rebrand/images/govuk-icon-180.pngis excluded by!**/*.pngapplication/static/govuk/assets/rebrand/images/govuk-icon-192.pngis excluded by!**/*.pngapplication/static/govuk/assets/rebrand/images/govuk-icon-512.pngis excluded by!**/*.pngapplication/static/govuk/assets/rebrand/images/govuk-icon-mask.svgis excluded by!**/*.svgapplication/static/govuk/assets/rebrand/images/govuk-opengraph-image.pngis excluded by!**/*.pngapplication/static/images/dl-apple-touch-icon-152.pngis excluded by!**/*.pngapplication/static/images/dl-apple-touch-icon-167.pngis excluded by!**/*.pngapplication/static/images/dl-apple-touch-icon-180.pngis excluded by!**/*.pngapplication/static/images/dl-apple-touch-icon-192.pngis excluded by!**/*.pngapplication/static/images/dl-apple-touch-icon-196.pngis excluded by!**/*.pngapplication/static/images/dl-apple-touch-icon.pngis excluded by!**/*.pngapplication/static/images/favicon-32.pngis excluded by!**/*.pngapplication/static/images/favicon.icois excluded by!**/*.icoapplication/static/images/favicon.svgis excluded by!**/*.svgapplication/static/images/opengraph.pngis excluded by!**/*.pngapplication/static/javascripts/vendor/accessible-autocomplete.min.jsis excluded by!**/*.min.jsapplication/static/javascripts/vendor/moj-frontend.js.mapis excluded by!**/*.mapapplication/static/stylesheets/application.css.mapis excluded by!**/*.mapapplication/static/stylesheets/vendor/accessible-autocomplete.css.mapis excluded by!**/*.mapapplication/static/stylesheets/vendor/moj-frontend.min.cssis excluded by!**/*.min.cssapplication/static/stylesheets/vendor/moj-frontend.min.css.mapis excluded by!**/*.mappackage-lock.jsonis excluded by!**/package-lock.jsonsrc/scss/vendor/moj-frontend.min.cssis excluded by!**/*.min.csssrc/scss/vendor/moj-frontend.min.css.mapis excluded by!**/*.map
📒 Files selected for processing (28)
.gitignore.npmrc.nvmrc.python-versionDockerfileMakefileapplication/static/govuk/assets/manifest.jsonapplication/static/govuk/assets/rebrand/manifest.jsonapplication/static/javascripts/Leaflet.recentre.jsapplication/static/javascripts/application.jsapplication/static/javascripts/base-tile.jsonapplication/static/javascripts/digital-land-cookies.jsapplication/static/javascripts/digital-land-frontend.jsapplication/static/javascripts/digital-land-maps.jsapplication/static/javascripts/govuk/govuk-frontend.jsapplication/static/javascripts/modules/cookies.jsapplication/static/javascripts/modules/multi-select-or-new.jsapplication/static/javascripts/modules/multi-select.jsapplication/static/javascripts/modules/select-or-new.jsapplication/static/javascripts/modules/utils.jsapplication/static/javascripts/utils.jsapplication/static/javascripts/vendor/moj-frontend.jsapplication/static/stylesheets/application.cssapplication/static/stylesheets/vendor/accessible-autocomplete.csspackage.jsonrequirements/dev-requirements.txtrequirements/requirements.txtsrc/javascripts/vendor/moj-frontend.js
💤 Files with no reviewable changes (16)
- application/static/govuk/assets/manifest.json
- application/static/govuk/assets/rebrand/manifest.json
- application/static/stylesheets/vendor/accessible-autocomplete.css
- application/static/javascripts/utils.js
- application/static/javascripts/modules/multi-select.js
- application/static/javascripts/modules/select-or-new.js
- requirements/requirements.txt
- application/static/javascripts/modules/multi-select-or-new.js
- .npmrc
- application/static/javascripts/application.js
- application/static/javascripts/digital-land-frontend.js
- application/static/javascripts/modules/utils.js
- application/static/javascripts/digital-land-cookies.js
- application/static/javascripts/digital-land-maps.js
- application/static/javascripts/modules/cookies.js
- application/static/javascripts/Leaflet.recentre.js
| COPY . . | ||
| COPY --from=assets /app/application/static ./application/static | ||
|
|
||
| RUN pip install -r requirements/requirements.txt | ||
| RUN pip install -r requirements/dev-requirements.txt |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Optimise Docker cache by moving dependency installation before copying source code.
Currently, pip install runs after COPY . .. This means any change to your source code (like editing a .js or .py file) will break the Docker build cache for the Python dependencies, forcing them to re-install on every build.
Copy the requirements files and run pip install before copying the rest of the application code to significantly speed up your build times.
⚡ Proposed fix for better layer caching
+COPY requirements/ ./requirements/
+RUN pip install -r requirements/requirements.txt
+RUN pip install -r requirements/dev-requirements.txt
+
COPY . .
COPY --from=assets /app/application/static ./application/static
-
-RUN pip install -r requirements/requirements.txt
-RUN pip install -r requirements/dev-requirements.txt📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| COPY . . | |
| COPY --from=assets /app/application/static ./application/static | |
| RUN pip install -r requirements/requirements.txt | |
| RUN pip install -r requirements/dev-requirements.txt | |
| COPY requirements/ ./requirements/ | |
| RUN pip install -r requirements/requirements.txt | |
| RUN pip install -r requirements/dev-requirements.txt | |
| COPY . . | |
| COPY --from=assets /app/application/static ./application/static |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Dockerfile` around lines 38 - 42, Reorder the Dockerfile dependency layers so
the requirements files are copied and both pip install commands run before the
broad COPY . . step. Keep the static assets copy and application source copy
after dependency installation, preserving the existing requirements paths and
install behavior while allowing source changes to reuse the dependency cache.
| "copy:moj-frontend-js": "rsync -ar ./node_modules/@ministryofjustice/frontend/moj/all.bundle.js application/static/javascripts/vendor/moj-frontend.js && rsync -ar ./node_modules/@ministryofjustice/frontend/moj/all.bundle.js.map application/static/javascripts/vendor/", | ||
| "copy:moj-frontend-css": "rsync -ar ./node_modules/@ministryofjustice/frontend/moj/moj-frontend.min.css* application/static/stylesheets/vendor/", | ||
| "copy:moj-frontend": "npm run copy:moj-frontend-js && npm run copy:moj-frontend-css", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if vendor source directories exist
ls -d src/javascripts/vendor 2>/dev/null || echo "javascripts/vendor does not exist"
ls -d src/stylesheets/vendor 2>/dev/null || echo "stylesheets/vendor does not exist"Repository: digital-land/planning-data-design
Length of output: 228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n[package.json scripts]\n'
cat -n package.json | sed -n '1,80p'
printf '\n[relevant paths]\n'
git ls-files 'application/static/javascripts/vendor' 'application/static/stylesheets/vendor' 'src/javascripts/vendor' 'src/stylesheets/vendor'
printf '\n[rsync destination probe]\n'
tmpdir="$(mktemp -d)"
mkdir -p "$tmpdir/src"
printf 'x' > "$tmpdir/src/file.txt"
# Try a file copy into a missing destination directory.
if rsync -ar "$tmpdir/src/file.txt" "$tmpdir/destdir/file.txt" >/tmp/rsync_probe.out 2>/tmp/rsync_probe.err; then
echo "rsync_to_missing_dest=success"
else
echo "rsync_to_missing_dest=failed"
fi
echo "--- stdout ---"
cat /tmp/rsync_probe.out
echo "--- stderr ---"
cat /tmp/rsync_probe.err
echo "--- dest tree ---"
find "$tmpdir" -maxdepth 3 -type f | sortRepository: digital-land/planning-data-design
Length of output: 2724
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '[tracked vendor paths under application/static]\n'
git ls-files 'application/static/**/vendor/**' | sed -n '1,120p'
printf '\n[files mentioning build.stylesheets or stylesheet vendor]\n'
rg -n '"build\.stylesheets"|stylesheets/vendor|application/static/stylesheets' package.json . -g '!node_modules' -g '!dist' -g '!build'
printf '\n[outline of likely package scripts source if present]\n'
fd -a 'package-scripts.js' .Repository: digital-land/planning-data-design
Length of output: 1176
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '[Dockerfile]\n'
cat -n Dockerfile | sed -n '1,120p'
printf '\n[Dockerfile.assets]\n'
cat -n Dockerfile.assets | sed -n '1,120p'
printf '\n[package-scripts / config references]\n'
rg -n 'build\.stylesheets|copy\.javascripts|copy\.images|vendor|mkdir -p|rsync -ar' package.json digital-land-frontend.config.json application -g '!node_modules' -g '!dist' -g '!build'Repository: digital-land/planning-data-design
Length of output: 4188
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n digital-land-frontend.config.jsonRepository: digital-land/planning-data-design
Length of output: 535
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'application/static/stylesheets/vendor|stylesheets/vendor|javascripts/vendor' . -g '!node_modules' -g '!dist' -g '!build'Repository: digital-land/planning-data-design
Length of output: 1373
🌐 Web query:
rsync create nested destination parent directories when copying a file to a non-existent path
💡 Result:
To create missing nested parent directories when copying a file with rsync, the recommended approach is to use the --mkpath option [1][2]. The --mkpath option was introduced in rsync version 3.2.3 to explicitly handle this scenario [3]. When this flag is used, rsync creates all missing destination path components, functioning similarly to running mkdir -p on the receiving side [1][4]. For example: rsync --mkpath source_file user@remote:/path/to/non/existent/directory/ If you are using a version of rsync older than 3.2.3, you cannot rely on a single native flag to create arbitrary missing parent directories. In such cases, common workarounds include: 1. Pre-creating the directory: Run an SSH command to create the path before the rsync transfer [5][3]. ssh user@remote "mkdir -p /path/to/non/existent/directory/" rsync source_file user@remote:/path/to/non/existent/directory/ 2. Using the --relative (-R) option: This can be used to replicate source directory structures at the destination, though it behaves differently than --mkpath by including parts of the source path in the destination [1][2]. Note: By default, rsync is designed to permit only the final component of the destination path to be missing [1][6]. If you attempt to copy to a path with multiple missing parents without --mkpath or a workaround, the transfer will typically fail [3].
Citations:
- 1: https://rsync.samba.org/ftp/rsync/rsync.1
- 2: https://github.com/RsyncProject/rsync/blob/b6457bbc839b2e02e7fd34ad5c1ddd7f23ee02d3/rsync.1.md
- 3: https://stackoverflow.com/questions/18491548/rsync-create-all-missing-parent-directories
- 4: https://download.samba.org/pub/rsync/rsync.1
- 5: https://stackoverflow.com/questions/1636889/how-can-i-configure-rsync-to-create-target-directory-on-remote-server
- 6: https://github.com/RsyncProject/rsync/blob/a4b926dcdce96b0f2cc0dc7744e95747b233500a/rsync.1.md
Create application/static/stylesheets/vendor before copying MOJ CSS. copyjs already seeds application/static/javascripts/vendor, but nothing creates the stylesheet vendor directory, so this rsync can fail on a clean build.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` around lines 18 - 20, Update the copy:moj-frontend-css script
to create application/static/stylesheets/vendor before running rsync, ensuring
the CSS copy succeeds on a clean build. Keep the existing CSS source and
destination behavior unchanged.
1 participant
What type of PR is this? (check all applicable)
Description
maintenance PR aimed at upgrading authlib, stumbled upon bad front end static management that needed fixing
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Please replace this line with instructions on how to test your changes, a note
on the devices and browsers this has been tested on, as well as any relevant
images for UI changes.
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above. Please refer to the Digital Land Testing Guidance for more information.
have not been included
[optional] Are there any post deployment tasks we need to perform?
[optional] Are there any dependencies on other PRs or Work?
Summary by CodeRabbit