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

Adds Docker files as a milestone for issue-4 #65

Merged
merged 1 commit into from
Nov 9, 2019
Merged

Adds Docker files as a milestone for issue-4 #65

merged 1 commit into from
Nov 9, 2019

Conversation

Reza-Rajabi
Copy link
Contributor

@Reza-Rajabi Reza-Rajabi commented Nov 7, 2019

What is this pull request:
This will add Dockerfile and .dockerignore to the project. So that we can run the project without installing node packages like so:

$ docker-compose up --build

and more importantly, we can ship the Docker image to Kubernetes later on.

@Reza-Rajabi Reza-Rajabi mentioned this pull request Nov 7, 2019
5 tasks
@manekenpix
Copy link
Member

@Reza-Rajabi is your problem related to issue #48?

@Reza-Rajabi
Copy link
Contributor Author

@manekenpix Yes it is. Thank you for catching that. I didn't include Redis.

@manekenpix
Copy link
Member

manekenpix commented Nov 7, 2019

@humphd mentioned in #4:

See https://hub.docker.com/_/redis. We can just pull existing images for it.

I tried a docker-compose file that pulls an existing redis image and it works. Feel free to add it to your PR:

version: '3'
services:
  telescope:
    build: .
    depends_on: 
      - redis
    ports:
      - "3000:3000"
    network_mode: host
  redis:
    image: redis:latest
    ports:
      - "6379:6379"

docker-compose up --build for deployment

@manekenpix manekenpix self-requested a review November 7, 2019 06:45
@Reza-Rajabi
Copy link
Contributor Author

Reza-Rajabi commented Nov 7, 2019

@manekenpix That's true. Thank you Josue, always helpful. I will add that compose file.

Copy link
Member

@manekenpix manekenpix left a comment

Choose a reason for hiding this comment

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

I think it looks great, maybe you could include some minimal info in README.md about deployment, and mentioned @jerryshueh's comment on slack about docker and Windows.

@birtony
Copy link
Contributor

birtony commented Nov 7, 2019

@Reza-Rajabi if this PR fixes #4, could you, please, update the title and the description accordingly?

@Reza-Rajabi
Copy link
Contributor Author

@Reza-Rajabi if this PR fixes #4, could you, please, update the title and the description accordingly?

@birtony Thank you for catching this. Your point is totally true. The reason I didn't follow this style is that we are not done with this issue, and the commit is actually one milestone of maybe 4 milestones or more. I appreciate your attention. Please keep watching to keep the project clean together.

@Reza-Rajabi
Copy link
Contributor Author

@manekenpix Do I need to change something in here? I will add the document once it gets merged though.
I need one more review as well and I don't like to assign it to somebody myself, do you have any suggestion for that?

@manekenpix
Copy link
Member

I think it'd be better to add the changes to the document here, that way we can have all the initial commits related to docker in this PR and it'll be easier to review the history of the repo in the future. What do you think?

Other than that, ready to get merged 👍

@Reza-Rajabi
Copy link
Contributor Author

I think it'd be better to add the changes to the document here, that way we can have all the initial commits related to docker in this PR and it'll be easier to review the history of the repo in the future. What do you think?

Other than that, ready to get merged 👍

true. I will make a commit to address the doc change.

@Reza-Rajabi
Copy link
Contributor Author

@manekenpix I somewhat rebased. So only see the last commit for all changes related to Docker including the CONTRIBUTING.md

@manekenpix

This comment has been minimized.

@humphd
Copy link
Contributor

humphd commented Nov 8, 2019

@Reza-Rajabi it looks to me like you can just reset your issue-4-docker branch to the 6c1aa4f commit and rebase:

git checkout -B issue-4-docker 6c1aa4f125c5d2b562c8439880ccebac2e18dd2a
git checkout master
git pull upstream master
git checkout issue-4-docker
git rebase master
git push origin issue-4-docker -f

We should make sure people don't merge with master ever, only rebase.

@Reza-Rajabi Reza-Rajabi closed this Nov 8, 2019
@Reza-Rajabi
Copy link
Contributor Author

@humphd @manekenpix My apologies! I tried to clean that mess and now the pull request got closed. So do I need to reopen this pull request or I need to make a new pull request?

@manekenpix
Copy link
Member

@Reza-Rajabi you can just reopen it. No worries 👍

@Reza-Rajabi Reza-Rajabi reopened this Nov 8, 2019
@Reza-Rajabi Reza-Rajabi closed this Nov 8, 2019
@Reza-Rajabi Reza-Rajabi reopened this Nov 9, 2019
@Reza-Rajabi
Copy link
Contributor Author

@manekenpix I will add the doc changes after these gets merged. I don't want to risk on rebase again.

COPY . .

ENV NODE_ENV production
ENV PORT 3000
Copy link
Contributor

Choose a reason for hiding this comment

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

These should both work with our .env, and override via environment variables, but we should confirm, since we expect to be able to set them via .env

@humphd humphd merged commit 288fcfd into Seneca-CDOT:issue-4 Nov 9, 2019
@humphd
Copy link
Contributor

humphd commented Nov 9, 2019

@Reza-Rajabi it looks like this PR was made against the wrong branch, so I've merged it, but not into master. Should it have been against master? If so I'll merge it there.

@humphd humphd mentioned this pull request Nov 9, 2019
@humphd
Copy link
Contributor

humphd commented Nov 9, 2019

Opened #113 so we can fix this. Please comment there.

@Reza-Rajabi Reza-Rajabi deleted the issue-4-docker branch November 17, 2019 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants