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

Use src/index.js in Dockerfile and docker-compose.yml - Closes #3039 #3043

Merged
merged 2 commits into from Mar 13, 2019

Conversation

Projects
6 participants
@fchavant
Copy link
Member

commented Mar 11, 2019

What was the problem?

Dockerfile and docker/docker-compose.yml were pointing to app.js which has been renamed to src/index.js.

How did I fix it?

Modified the above-mentioned files.

How to test it?

Build docker image and run it; make sure the application starts properly.

Review checklist

  • The PR resolves #3039
  • All new code is covered with unit tests
  • All new code was formatted with Prettier
  • Linting passes
  • Tests pass
  • Commit messages follow the commit guidelines
  • Documentation has been added/updated

@fchavant fchavant self-assigned this Mar 11, 2019

@fchavant fchavant added this to In progress in Version 1.6.0 via automation Mar 11, 2019

@fchavant fchavant requested review from diego-G, psychopenguin and ManuGowda Mar 11, 2019

Version 1.6.0 automation moved this from In progress to Pending Review Mar 11, 2019

@ManuGowda

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

@fchavant we discussed in our last meeting about keeping it to npm start so that we can avoid such changes in the future. I agree that you mentioned about npm being included in the build and its an issue, but just wanted to get a clarification that we are considering that approach.

@fchavant

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2019

@ManuGowda I followed the instructions from #3039 and did not consider using npm start; what would be the benefit?

@ManuGowda

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

@ManuGowda I followed the instructions from #3039 and did not consider using npm start; what would be the benefit?

The benefit would be you need not worry in the future if they change it from index.js to something else and we can maintian the same in commander or wherever we use it. @shuse2 what's your take?

@shuse2

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

I think adding to npm start is good. it's a standard in node projects, and always have one entry point to the application

@fchavant

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2019

@ManuGowda @shuse2 now using npm start

@fchavant fchavant requested a review from diego-G Mar 11, 2019

@shuse2

shuse2 approved these changes Mar 11, 2019

@MaciejBaj MaciejBaj merged commit d010af3 into development Mar 13, 2019

3 checks passed

jenkins-ci/lisk-core This commit looks good
Details
jenkins-ci/lisk-core-network This commit looks good
Details
security/snyk - package.json (LiskHQ) No manifest changes detected

Version 1.6.0 automation moved this from Pending Review to Closed PRs Mar 13, 2019

@MaciejBaj MaciejBaj deleted the 3039-update-app-docker branch Mar 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.