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

Ran compose recipes + Move to NPM + port 8008 (Part 6) #147

Merged
merged 17 commits into from
Oct 28, 2023

Conversation

melroy89
Copy link
Member

@melroy89 melroy89 commented Oct 27, 2023

  • Clean-up clover.xml file (should have never been checked in the first place)
  • Move from Yarn to NPM (See Webpack Encore PR: Recommend NPM instead of Yarn symfony/recipes#1229)
    • Indirectly also updates all NodeJS packages to the latest version
  • I dare to run the recipe symfony/webpack-encore-bundle as well as doctrine/doctrine-bundle this time, but:
  • Update Docker locally mapped port from 80 to port 8008 (BOOB 🔒 )

@melroy89 melroy89 changed the title Improve error warning log pollution + Move to NPM + port 8008 (Part 6) Improve warnings + Move to NPM + port 8008 (Part 6) Oct 27, 2023
@melroy89 melroy89 changed the title Improve warnings + Move to NPM + port 8008 (Part 6) Ran compose recipes + Move to NPM + port 8008 (Part 6) Oct 27, 2023
@melroy89 melroy89 marked this pull request as ready for review October 27, 2023 23:36
@melroy89 melroy89 requested a review from a user October 27, 2023 23:37
@melroy89 melroy89 added documentation Improvements or additions to documentation enhancement New feature or request dependencies Pull requests that update a dependency file backend Backend related issues and pull requests docker Issues and pull requests related to docker environment labels Oct 28, 2023
@melroy89 melroy89 requested a review from e-five256 October 28, 2023 00:02
@e-five256
Copy link
Member

I'm having issues building docker for this with

 => ERROR [messenger_ap builder-nodejs 9/9] RUN npm run build                                                                                                                                                                            0.8s
------
 > [messenger_ap builder-nodejs 9/9] RUN npm run build:
0.743 
0.743 > build
0.743 > encore production --progress
0.743 
0.747 sh: encore: not found
------
failed to solve: process "/bin/sh -c npm run build" did not complete successfully: exit code: 127

@melroy89
Copy link
Member Author

melroy89 commented Oct 28, 2023

I'm having issues building docker for this with

 => ERROR [messenger_ap builder-nodejs 9/9] RUN npm run build                                                                                                                                                                            0.8s
------
 > [messenger_ap builder-nodejs 9/9] RUN npm run build:
0.743 
0.743 > build
0.743 > encore production --progress
0.743 
0.747 sh: encore: not found
------
failed to solve: process "/bin/sh -c npm run build" did not complete successfully: exit code: 127

Sorry about that, I tested Docker earlier, but I added NODE_ENV=production later I think. Anyhow, I include --include=dev now. I did the same on other places just to be sure. And also execute a npm run build with production env var on post-upgrade script.

@e-five256 try again 👍🏽

e-five256
e-five256 previously approved these changes Oct 28, 2023
Copy link
Member

@e-five256 e-five256 left a comment

Choose a reason for hiding this comment

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

this builds for my local dev env, and I didn't notice any issues, but I'm not as knowledgeable compared to my peers so not sure if you want a second opinion just to be cautious, since it seems like a pretty big change. I also think it's a good idea to communicate maybe in matrix general once merged as a heads up that the www container port on the host changed if people need to repoint things to it

@ghost
Copy link

ghost commented Oct 28, 2023

It looks like we don't need to use node:lts-alpine image to support whatever was broken with yarn, the latest release of node:alpine works fine now. I doubled checked that and all these changes on a docker dev instance with db test fixtures, everything still seems to work fine.

Edit: added commit with this change.

ghost

This comment was marked as outdated.

`node:lts-alpine` is no longer needed to support `yarn` issues, latest `node:alpine` works fine now with straight `npm`.
@ghost ghost requested review from e-five256 and a user October 28, 2023 13:32
@melroy89
Copy link
Member Author

Thanks for the improvement @nobodyatroot

@melroy89 melroy89 merged commit 3c67ee0 into main Oct 28, 2023
@melroy89 melroy89 deleted the improve_error_warning_log_pollution_6 branch October 28, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Backend related issues and pull requests dependencies Pull requests that update a dependency file docker Issues and pull requests related to docker environment documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants