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

chore(dev): add virus scanner npm install to root install command #7069

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

justynoh
Copy link
Contributor

@justynoh justynoh commented Feb 6, 2024

Problem

npm install in the root directory only performs package installation for the frontend and backend, even though npm run dev attempts to run the frontend, backend and virus scanner services.

Solution

Add virus scanner package installation into the root postinstall step.

Breaking Changes

  • No - this PR is backwards compatible

@KenLSM
Copy link
Contributor

KenLSM commented Feb 6, 2024

Im thinking that this shouldn't be added to root as virus-scanner is not required to build the image for formsg

@justynoh
Copy link
Contributor Author

justynoh commented Feb 6, 2024

hmm, but by that logic, the frontend install also shouldn't be included in root install right haha 😅 or am i missing something

@KenLSM
Copy link
Contributor

KenLSM commented Feb 6, 2024

ccmiw

github: build FormFE && FormBE
eb: upload FormFE to s3 (after env configs are in)
eb: run FormBE

Although I think we can install them separately after we have achieved the split deployment of FE & BE

@justynoh
Copy link
Contributor Author

justynoh commented Feb 6, 2024

oh, this was more from a DX perspective haha. if you clone the repo fresh, npm i npm run build:frontend and npm run dev, the last command fails because it attempts to start virus scanner without having installed it's packages first. not so much from deployment perspective

that being said, the npm i does influence deployment speed. But i think the number of packages in the virus scanner isn't that much, the impact to deployment should be negligible. in comparison, the frontend install takes significantly longer haha

@justynoh justynoh merged commit d77a7c3 into develop Feb 14, 2024
28 checks passed
@justynoh justynoh deleted the chore/add-virus-scanner-to-install branch February 14, 2024 02:04
@tshuli tshuli mentioned this pull request Feb 14, 2024
9 tasks
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