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

Feat/vite (front end) build #43

Merged
merged 9 commits into from
Sep 20, 2023
Merged

Feat/vite (front end) build #43

merged 9 commits into from
Sep 20, 2023

Conversation

sloan58
Copy link
Contributor

@sloan58 sloan58 commented Sep 19, 2023

If npm dependencies exist (package.json or package-lock.json), build front end assets.

@smortexa
Copy link
Member

@sloan58 The problem still exists. The tests fail.

@sloan58
Copy link
Contributor Author

sloan58 commented Sep 20, 2023

That's interesting. If you check the actions that ran when I pushed yesterday they all passed. I see that you adjusted the npm run build section. Was that before or after the tests ran?

@sloan58
Copy link
Contributor Author

sloan58 commented Sep 20, 2023

I think we need to leave this in there. If there's no package.json file, we shouldn't build. My local tests fail as well on the current codebase if I delete package.json

RUN if [ -f $ROOT/package.json ] || [ -f $ROOT/package-lock.json ]; \
  then \
  npm run build; \
  fi

@smortexa
Copy link
Member

I agree with you. After making the adjustments, but the tests failed. The if command was preventing the npm run build command from executing.

@sloan58
Copy link
Contributor Author

sloan58 commented Sep 20, 2023

With my latest commit I was able to build an image with or without a package.json file. Maybe there should be two additional tests (two with and two without package.json for swoole and rr)?

@smortexa
Copy link
Member

That's true. But I removed it to test that the build stage works correctly.

@sloan58
Copy link
Contributor Author

sloan58 commented Sep 20, 2023

I guess I'm not sure why we updated the if statement. My local tests and the Github actions passed with my latest commit, and if we do not build the frontend assets, they'll still get copied over to the final image since we're running a COPY . . regardless during the build stage.

@smortexa
Copy link
Member

By using the if statement, the image can be built successfully without encountering any errors. However, it's important to note that the npm run build command will not be executed, which means that our front-end assets will not be built even though the image is successfully built.

@sloan58
Copy link
Contributor Author

sloan58 commented Sep 20, 2023

Understood. If there's no package.json file we shouldn't run npm run build as it will fail. If we want to test the case that package.json doesn't exist, we could have two more tests that delete package.json prior to building the image.

@smortexa
Copy link
Member

Ahhhhhh, the package-lock.json and package.json files are listed in .dockerignore file!

@smortexa smortexa merged commit e7372a4 into exaco:main Sep 20, 2023
4 checks passed
@sloan58
Copy link
Contributor Author

sloan58 commented Sep 20, 2023

Awesome! Thanks for the help getting this merged!

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.

2 participants