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

Deploy with riff-raff #1693

Merged
merged 9 commits into from
Dec 23, 2024
Merged

Deploy with riff-raff #1693

merged 9 commits into from
Dec 23, 2024

Conversation

Jakeii
Copy link
Member

@Jakeii Jakeii commented Dec 5, 2024

What does this change?

Following on from the spike done by @chrislomaxjones a while back #857

Adds a riff-raff.yml with configuration for uploading commercial bundle to the frontend static s3 bucket and some cloudformation to update a key with the location of the commercial bundle for each stage.

This also adds a new webpack config to build a variant of the commercial bundle suitable for deploying in this way (simply a change to the webpack public path config), the new webpack config also creates the cloudformation to update the parameter store key with the hashed path of the bundle.

Corresponding Frontend PR guardian/frontend#27578 which will read the parameter store key.

Why?

Deploying commercial at the moment is a very arduous process with poor developer experience, it requires adding a changeset, releasing a version, waiting 3 times for checks to run in the commercial repo (feature PR, version bump PR and on main). Then bumping the version in frontend rebuilding and redeploying the entirety of it just to change a script on the page.

This allows our deploys to be almost immediately available!

Copy link

changeset-bot bot commented Dec 5, 2024

⚠️ No Changeset found

Latest commit: b008cb1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

github-actions bot commented Dec 5, 2024

Ad load time test results

For consented, top-above-nav took on average 5130ms to load.
For consentless, top-above-nav took on average 3347ms to load.

Test conditions:

  • 5mbps download speed
  • 1.5mbps upload speed
  • 150ms latency

@Jakeii Jakeii marked this pull request as ready for review December 5, 2024 16:43
@Jakeii Jakeii requested a review from a team as a code owner December 5, 2024 16:43
Comment on lines +20 to +23
"build": "npm-run-all clean --parallel compile:core:* build:prod build:dev build:riff-raff",
"build:dev": "webpack -c webpack.config.dev.mjs",
"build:prod": "webpack -c webpack.config.prod.mjs",
"build:riff-raff": "webpack -c webpack.config.riff-raff.mjs",
Copy link
Contributor

Choose a reason for hiding this comment

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

From my understanding, the build:prod script sets process.env.RIFFRAFF_DEPLOY to false to exclude Riff-Raff-specific code from the production build.
The build:riff-raff script sets process.env.RIFFRAFF_DEPLOY to true and includes additional steps for Riff-Raff deployment. Could you please clarify if this understanding is correct? which specific scenarios where each build would be used?

Copy link
Member

Choose a reason for hiding this comment

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

process.env.RIFFRAFF_DEPLOY is set by the webpack config used by each build task. The comment below on DefinePlugin might help.

@@ -39,6 +39,7 @@ export default merge(config, {
new DefinePlugin({
'process.env.NODE_ENV': JSON.stringify('production'),
'process.env.OVERRIDE_BUNDLE_PATH': JSON.stringify(false),
'process.env.RIFFRAFF_DEPLOY': JSON.stringify(false),
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we setting the global RIFFRAFF_DEPLOY to false to isolate the 2 build processes?

Copy link
Member

Choose a reason for hiding this comment

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

We use DefinePlugin to replace instances in our code of the strings on the left (the key) with the values on the right. It allows us to selectively override these values in the webpack config.

As an example you see how we configure process.env.OVERRIDE_BUNDLE_PATH in our dev vs prod webpack build here:

new DefinePlugin({
'process.env.OVERRIDE_BUNDLE_PATH':
JSON.stringify(overrideBundlePath),
}),

new DefinePlugin({
'process.env.NODE_ENV': JSON.stringify('production'),
'process.env.OVERRIDE_BUNDLE_PATH': JSON.stringify(false),
'process.env.RIFFRAFF_DEPLOY': JSON.stringify(false),
...gitCommitSHA(),
}),

Comment on lines +15 to +17
if (!process.env.RIFFRAFF_DEPLOY) {
__webpack_public_path__ = decideAssetsPath();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

From my understanding, this code sets __webpack_public_path__ at runtime to determine the base path for assets. If process.env.RIFFRAFF_DEPLOY is false, the public path is set to the CDN (https://assets.guim.co.uk/javascripts/commercial/). If process.env.RIFFRAFF_DEPLOY is true, the public path is managed by the RiffRaff deployment process, and the frontend application reads the parameter store key to include the appropriate <script> tag.

Could you confirm if this understanding is correct and provide any additional context?

Copy link
Member Author

@Jakeii Jakeii Dec 6, 2024

Choose a reason for hiding this comment

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

Both frontendAssetsFullURL and page.assetsPath gets set to /assets instead of assets(-code).guim.co.uk when running locally to serve frontends static assets locally. But as commercial would no longer be there that's no use.

By setting to auto here, chunks will use whatever the entrypoint uses.

Often we are overriding the bundle location completely locally so not a huge issue, but it's nice to have ads working even if not overriding the bundle location.

@Jakeii Jakeii requested a review from arelra December 6, 2024 11:27
Copy link
Member

@arelra arelra left a comment

Choose a reason for hiding this comment

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

Amazing work @Jakeii ✨ ✨ ✨

How this works end-to-end with Frontend might benefit from some documentation but that could be a follow up task.

@Jakeii Jakeii merged commit 1d81a68 into main Dec 23, 2024
13 of 14 checks passed
@Jakeii Jakeii deleted the jlk/deploy-with-riff-raff branch December 23, 2024 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants