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

Try parallel app processing #73557

Merged
merged 6 commits into from Mar 15, 2023
Merged

Try parallel app processing #73557

merged 6 commits into from Mar 15, 2023

Conversation

noahtallen
Copy link
Member

@noahtallen noahtallen commented Feb 20, 2023

This PR creates an all-in-one Calypso app artifact processor. In the previous approach, with each app processed in an individual TeamCity build, the artifact process script only had to care about one app.

The major change here is to parallelize and combine in the following ways:

  1. In parallel, download the artifact for each app and compare it to the release. This gives us a Promise.all over functions which return boolean, which in turn gives us a list of apps which are changing.
  2. Update the tag logic to do multiple tags in a build, based on the changing apps.
  3. Update the notification logic to handle multiple apps. Instead of one GH comment per app, we'll have one comment which lists the app. (The slack endpoint will ultimately have to be updated too, but it's currently unused.)

All of this is re-written in NodeJS to be (hopefully!) easier to read than bash script in kotlin string! It's also easier to manage data and parallelization.

I've tested the following parts:

  1. Does it detect a changing app vs previous tagged build: ✅
  2. Does it add and remove GH comment: ✅
  3. Does it authenticate with the Slack endpoint: ✅
  4. Does it add tags based on what changes: ✅

What's missing: the previous artifact processor added a build_meta.txt file. I'd like to deprecate that in favor of build_meta.json. Some builds generate that with webpack, but we need some way to get it in all builds.

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@matticbot
Copy link
Contributor

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • happy-blocks

To test WordPress.com changes, run "install-plugin.sh try-parallel-app-processing" on your sandbox.

@noahtallen noahtallen self-assigned this Feb 22, 2023
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 22, 2023
@@ -22,7 +22,7 @@
"start": "yarn run clean && yarn run build:notifications && yarn run dev-server",
"dev": "yarn run calypso-apps-builder --localPath dist --remotePath /home/wpcom/public_html/widgets.wp.com/notifications",
"build": "NODE_ENV=production yarn dev",
"build-ci": "yarn build && ./bin/prepArtifactForCI.sh"
Copy link
Member Author

Choose a reason for hiding this comment

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

Prep artifact actually requires the "prev build" to be downloaded already, so it should be in the normalization script

@worldomonation
Copy link
Contributor

worldomonation commented Feb 24, 2023

What's missing: the previous artifact processor added a build_meta.txt file. I'd like to deprecate that in favor of build_meta.json. Some builds generate that with webpack, but we need some way to get it in all builds.

I spun up a PR to hopefully address that. I didn't want to do it in the main stream of the CI optimization work, because (IIUC) the changeover to build_meta.json isn't absolutely necessary to complete the work.

@worldomonation
Copy link
Contributor

@noahtallen This PR is still in draft, but I think it's ready to review and possibly merge. What do you think?

@noahtallen
Copy link
Member Author

I'll take a look at the CI error and get it ready! You're probably right, we can test it out more after merging and getting more apps set up with it.

new webpack.NormalModuleReplacementPlugin(
package,
pkg,
Copy link
Member Author

Choose a reason for hiding this comment

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

Weird issue, but can consistently replicate when running the translation script in happy blocks of all places

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

done

# Add the index.php file
cp ./index.php ./README.md ./dist/* ./release-files/
cp ./index.php ./README.md ./release-files/
Copy link
Member Author

Choose a reason for hiding this comment

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

dist isn't used anymore, which can be verified by checking the trunk artifacts

@noahtallen noahtallen marked this pull request as ready for review March 15, 2023 01:34
@noahtallen noahtallen requested a review from a team March 15, 2023 01:34
@matticbot
Copy link
Contributor

This PR modifies the release build for happy-blocks

To test your changes on WordPress.com, run install-plugin.sh happy-blocks try-parallel-app-processing on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-r7r-p2

mkdir -p ./release-files/${block//\.\.\//};
cp -r $block/build/* ./release-files/${block//\.\.\//}/;
mkdir -p "./release-files/${block//\.\.\//}";
cp -r $block/build/* "./release-files/${block//\.\.\//}/";
Copy link
Member Author

Choose a reason for hiding this comment

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

I have a feeling this code shouldn't be in this script. It seems necessary for the plugin to work normally, but it's in a CI-only script. So the normal yarn build script won't work

@worldomonation I wonder if this means we should move the CI related stuff back to .teamcity, so it's clearer that it should only be used for CI workflows, not for local builds 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

@noahtallen It's been close to a month so my recall is a bit hazy but I do think we can remove all build-ci yarn scripts, prepArtifactForCI.sh bash scripts and such from the repo for now. IIRC I tried to incorporate the processing into TeamCity in a follow-up branch I had.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should I push to your branch to do that?

Copy link
Member Author

Choose a reason for hiding this comment

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

ok cool! I don't think we need to do it in this branch. If we merge this separately we can split that up into other PRs which are easier to look at individually

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. I'll rebase my WIP branch after we merge, and get rid of these files and scripts.

@noahtallen noahtallen merged commit f6ad0f6 into trunk Mar 15, 2023
19 of 20 checks passed
@noahtallen noahtallen deleted the try-parallel-app-processing branch March 15, 2023 21:40
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 15, 2023
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.

None yet

3 participants