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

Nginx/Node/Redis and Flask/Redis Compose example #222

Merged
merged 70 commits into from
Mar 17, 2022

Conversation

ajeetraina
Copy link
Contributor

@ajeetraina ajeetraina commented Mar 5, 2022

Here's a single PR for multiple docker-compose sample projects that implements visitor counter powered by the following stack -

Project #1

  • NodeJS
  • Nginx proxy &
  • Redis

Project #2

  • Redis
  • Flask

Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Signed-off-by: Ajeet Singh Raina <ajeetraina@gmail.com>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
@ajeetraina ajeetraina changed the title Nginx/Node/Redis example Nginx/Node/Redis and Flask/Redis Compose example Mar 7, 2022
@glours
Copy link
Collaborator

glours commented Mar 9, 2022

Hello @ajeetraina
Can you sign your last commit please?

Signed-off-by: Ajeet Singh Raina <ajeetraina@gmail.com>
Copy link
Collaborator

@glours glours left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @ajeetraina

Please don't remove existing sample even if your contribution provide a more complex example.
Next time can you make a PR per sample? It's easier to review and merge, and it avoids to block your whole samples if only one need some changes.
Can you remove the unnecessary nginx-node-redis directory?
You also need to reference the new samples in the main README.md file

Regarding the nginx-node-redis sample, I think it should make more sens to only have one directory with the build of the node backend:

  • The image will be build only once
  • Using the hostname property, resolved by compose DNS, in your js code will be efficient to replace your hardcoded res.send('web1: Total number of visits is: ' + numVisitsToDisplay);

flask-redis/README.md Outdated Show resolved Hide resolved
flask-redis/app.py Outdated Show resolved Hide resolved
flask-redis/docker-compose.yml Outdated Show resolved Hide resolved
flask-redis/README.md Outdated Show resolved Hide resolved
nginx-nodejs-redis/README.md Outdated Show resolved Hide resolved
nginx-nodejs-redis/README.md Outdated Show resolved Hide resolved
flask-redis/Dockerfile Outdated Show resolved Hide resolved
nginx-nodejs-redis/nginx/Dockerfile Outdated Show resolved Hide resolved
nginx-nodejs-redis/web/Dockerfile Outdated Show resolved Hide resolved
nginx-nodejs-redis/web1/Dockerfile Outdated Show resolved Hide resolved
flask-redis/README.md Outdated Show resolved Hide resolved
@ajeetraina
Copy link
Contributor Author

Hi @glours I have made the changes. Let me know if it looks good.

Copy link
Collaborator

@glours glours left a comment

Choose a reason for hiding this comment

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

@ajeetraina I can't merge this one, the Flask single service sample is still deleted in the PR 😞
I proposed you some changes to simplify the 2nd sample and remove duplicate code

nginx-nodejs-redis/README.md Outdated Show resolved Hide resolved
nginx-nodejs-redis/docker-compose.yml Outdated Show resolved Hide resolved
nginx-nodejs-redis/web/server.js Outdated Show resolved Hide resolved
nginx-nodejs-redis/README.md Outdated Show resolved Hide resolved
nginx-nodejs-redis/docker-compose.yml Outdated Show resolved Hide resolved
nginx-nodejs-redis/docker-compose.yml Outdated Show resolved Hide resolved
nginx-nodejs-redis/web1/Dockerfile Outdated Show resolved Hide resolved
nginx-nodejs-redis/web2/Dockerfile Outdated Show resolved Hide resolved
flask-redis/README.md Outdated Show resolved Hide resolved
@glours
Copy link
Collaborator

glours commented Mar 12, 2022

You also need to sign all your commits and not only the last one, my bad

To add your Signed-off-by line to every commit in this branch:

Ensure you have a local copy of your branch by [checking out the pull request locally via command line](https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/checking-out-pull-requests-locally).
In your local branch, run: git rebase HEAD~21 --signoff
Force push your changes to overwrite the branch: git push --force-with-lease origin master

ajeetraina and others added 22 commits March 13, 2022 17:02
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Signed-off-by: Ajeet Singh Raina <ajeetraina@gmail.com>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Signed-off-by: Guillaume Lours <guillaume.lours@docker.com>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Co-authored-by: Guillaume Lours <guillaume@lours.me>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Co-authored-by: Guillaume Lours <guillaume@lours.me>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Co-authored-by: Guillaume Lours <guillaume@lours.me>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Signed-off-by: Ajeet Singh Raina <ajeetraina@gmail.com>
Signed-off-by: ajeetraina <ajeetraina@gmail.com>
Signed-off-by: Ajeet Singh Raina <ajeetraina@gmail.com>
@ajeetraina
Copy link
Contributor Author

ajeetraina commented Mar 13, 2022

I checked git log and it does show that it is signed-off. I am not sure why its not working.



commit c869829e37c89cffaa7e2370b6af47032e4062ff
Author: ajeetraina <ajeetraina@gmail.com>
Date:   Sun Mar 13 16:58:41 2022 +0530

    Restored single service Flask project

    Signed-off-by: Ajeet Singh Raina <ajeetraina@gmail.com>

Copy link
Collaborator

@glours glours left a comment

Choose a reason for hiding this comment

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

You're close to merge it!
I'm sorry I didn't saw improvements we can do on the Dockerfiles

nginx-nodejs-redis/web/Dockerfile Outdated Show resolved Hide resolved
flask-redis/Dockerfile Outdated Show resolved Hide resolved
ajeetraina and others added 2 commits March 15, 2022 16:47
Co-authored-by: Guillaume Lours <guillaume@lours.me>
Co-authored-by: Guillaume Lours <guillaume@lours.me>
Copy link
Collaborator

@glours glours left a comment

Choose a reason for hiding this comment

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

😬 the sample isn't starting due to a javascript typo

nginx-nodejs-redis/web/server.js Outdated Show resolved Hide resolved
Co-authored-by: Guillaume Lours <guillaume@lours.me>
Copy link
Collaborator

@glours glours left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks again for your contribution

@glours glours merged commit 04f8c9c into docker:master Mar 17, 2022
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