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.

Sign up for GitHub

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

Jump to bottom

Speed up review app installation #1870

Merged
merged 3 commits into from
Oct 21, 2024
Merged

Speed up review app installation #1870

merged 3 commits into from
Oct 21, 2024

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Oct 18, 2024 •
edited
Loading

Description of proposed changes

Reduce the time to create the preview PR from ~11m to ~3m.

Note

I plan to drop e4adf4f since it doesn't seem to have any effect: compare runs before and after
EDIT: keeping it as 2aba0cf

Related issue(s)

Checklist

@victorlin victorlin self-assigned this Oct 18, 2024
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-victorlin-updat-vycryt October 18, 2024 00:11 Inactive
@victorlin victorlin temporarily deployed to auspice-victorlin-updat-vycryt October 18, 2024 00:26 Inactive
@nextstrain-bot nextstrain-bot mentioned this pull request Oct 18, 2024
Closed
@victorlin victorlin mentioned this pull request Oct 18, 2024
Closed
@victorlin
Use Node.js version 20
13a105f 
This change doesn't really affect the installation speed, but it's good
to be consistent with the version used to build both nextstrain.org and
auspice.us.
@victorlin victorlin force-pushed the victorlin/update-review-app-installation branch from e7f3d0d to 13a105f Compare October 18, 2024 20:41
@victorlin victorlin temporarily deployed to auspice-victorlin-updat-vycryt October 18, 2024 20:41 Inactive
@victorlin
Don't run npm scripts
c90a465 
This prevents running Auspice's prepare script, which is not necessary
for installation without usage in the same environment.
@victorlin victorlin temporarily deployed to auspice-victorlin-updat-vycryt October 18, 2024 20:53 Inactive
@victorlin victorlin changed the title Update review app installation Speed up review app installation Oct 18, 2024
@victorlin victorlin marked this pull request as ready for review October 18, 2024 21:28
jameshadfield
jameshadfield approved these changes Oct 20, 2024
View reviewed changes
Copy link
Member

@jameshadfield jameshadfield left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Why drop e4adf4f? Its commit message provides a compelling reason to keep it...

@victorlin
Copy link
Member Author

@jameshadfield I forgot to explain in detail – I added that commit thinking it would ignore installation of Auspice devDependencies, but it only applies to devDependencies in the current project, i.e. nextstrain.org/auspice-client/package.json which has no devDependencies at the moment. You have a good point though – if that every changes in the future, we would still want to ignore devDependencies during installation for the review app. I'll keep the commit with a revised message.

Side note: the reason that Auspice devDependencies were installed and causing slowness was due to having a prepare script on a git install. NODE_ENV=production has no effect in this case. It is addressed with c90a465 and nextstrain/nextstrain.org@c3d38f2 elsewhere.

@victorlin
Set NODE_ENV=production during installation
2aba0cf 
This prevents installation of devDependencies in the current project.
There are none at the moment for both nextstrain.org and auspice.us, but
if that ever changes in the future, we would want to ignore them in this
step.
@victorlin victorlin force-pushed the victorlin/update-review-app-installation branch from e4adf4f to 2aba0cf Compare October 21, 2024 15:16
@victorlin victorlin temporarily deployed to auspice-victorlin-updat-vycryt October 21, 2024 15:16 Inactive
@victorlin victorlin merged commit 40beaf0 into master Oct 21, 2024
24 checks passed
@victorlin victorlin deleted the victorlin/update-review-app-installation branch October 21, 2024 15:18
@nextstrain-bot nextstrain-bot mentioned this pull request Oct 21, 2024
Closed
@victorlin
Copy link
Member Author

^ new preview PR is a bug. Will create a PR to fix.

@victorlin victorlin mentioned this pull request Oct 21, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jameshadfield jameshadfield jameshadfield approved these changes

Assignees

@victorlin victorlin

Labels
preview on nextstrain.org
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@victorlin @jameshadfield @nextstrain-bot