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

chore: add docker file #2179

Merged
merged 20 commits into from
Dec 19, 2023
Merged

Conversation

captain-Akshay
Copy link
Contributor

issue #2164
Description

  • added a .dockerignore to exclude files and directories from the context that is sent to the Docker container
  • added a Dockerfile for production environment
  • added a dockerfile for development environment

@netlify
Copy link

netlify bot commented Sep 28, 2023

Deploy Preview for asyncapi-website ready!

Name Link
🔨 Latest commit 9948007
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/6581903580e0890008a83eda
😎 Deploy Preview https://deploy-preview-2179--asyncapi-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions
Copy link

github-actions bot commented Sep 28, 2023

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 26
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🔴 PWA 30

Lighthouse ran on https://deploy-preview-2179--asyncapi-website.netlify.app/

Signed-off-by: captain-Akshay <[email protected]>
@captain-Akshay captain-Akshay changed the title enhancement:- added docker file feat: added docker file Sep 28, 2023
@akshatnema akshatnema linked an issue Sep 30, 2023 that may be closed by this pull request
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

it is enough to have a dockerfile just for development environment

please update PR by adding to the README and nice copy/paste instruction for contributors that explains how to build and run the image, and develop website alongside, seeing localhost preview regeneration

@captain-Akshay
Copy link
Contributor Author

captain-Akshay commented Oct 3, 2023

@derberg Ok sure I will add the documentation for the contributor where they can copy paste to start the docker container instantly

@akshatnema
Copy link
Member

@captain-Akshay Please make sure you add alpine image installation via docker in the system, using the following command:

 sudo docker pull alpine

Without this pre-requisite command, users will not able to build the website for first time.

@captain-Akshay
Copy link
Contributor Author

@akshatnema yeah i will make sure i make it as concise as possible maybe add a entrypoint.sh for user so that they dont have to type anything else other than ./entrypoint.sh

@derberg
Copy link
Member

derberg commented Oct 4, 2023

sudo docker pull alpine

@akshatnema are you sure? docker build works in the way that if image is not present, it pulls it by default

@akshatnema
Copy link
Member

@akshatnema are you sure? docker build works in the way that if image is not present, it pulls it by default

I will check once again. Although, when I did locally first time on my system, I got some errors, and after installing alpine, I got no more errors in it.

@captain-Akshay captain-Akshay requested a review from derberg October 7, 2023 01:59
@derberg
Copy link
Member

derberg commented Oct 7, 2023

@captain-Akshay

  • we need only one docker file, for development, no need for production
  • do you think we really need entrypoint script, why not just have docker commands in readme? especially that in case of windows you mention them

@captain-Akshay
Copy link
Contributor Author

@captain-Akshay

* we need only one docker file, for development, no need for production

* do you think we really need entrypoint script, why not just have docker commands in readme? especially that in case of windows you mention them

true! i m thinking way ahead sorry @derberg 😅

README.md Outdated

### For Windows
- `cd` into the repo
- run the command one by one `docker build -t async_image -f Dockerfile.dev .` then `docker run -d -p 3000:3000 async_image`
Copy link
Member

Choose a reason for hiding this comment

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

I guess we do not need windows specific instructions, right? also dockerfile.dev no longer here

README.md Outdated Show resolved Hide resolved
README.md Outdated

- `cd` into the repo
- run the following command `docker build -t async_image -f Dockerfile . && docker run -d -p 3000:3000 async_image`
- Visit localhost:3000 and the website should be live
Copy link
Member

Choose a reason for hiding this comment

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

ok, so we know how to build, and how to run, what about development - this part is critical.

I want to run dev environment for development, so I need to run an image mapping local fork of the repo, right? so I can develop in my IDE, make changes, and rebuild and refresh of website is still possible - through docker this time

@captain-Akshay captain-Akshay requested a review from derberg October 9, 2023 10:35
@captain-Akshay
Copy link
Contributor Author

@derberg, I must express my admiration for your thought process and consideration of all conceivable scenarios is truly commendable, and I find it to be a valuable learning opportunity.

README.md Outdated

### Steps to start server

- `cd` into the repo
Copy link
Member

Choose a reason for hiding this comment

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

Instead of writing instruction cd into the repo, we can direct users "to go inside the directory of the repo and run the following commands."

Signed-off-by: captain-Akshay <[email protected]>
akshatnema
akshatnema previously approved these changes Oct 16, 2023
Copy link
Member

@akshatnema akshatnema left a comment

Choose a reason for hiding this comment

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

LGTM ✔️ . @derberg any points remaining?

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

left some comments, we need better readme

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
captain-Akshay and others added 5 commits October 27, 2023 16:35
Co-authored-by: Lukasz Gornicki <[email protected]>
Co-authored-by: Lukasz Gornicki <[email protected]>
Co-authored-by: Lukasz Gornicki <[email protected]>
Co-authored-by: Lukasz Gornicki <[email protected]>
Signed-off-by: captain-Akshay <[email protected]>
@asyncapi-bot
Copy link
Contributor

asyncapi-bot commented Dec 7, 2023

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 34
🟢 Accessibility 98
🟢 Best practices 92
🟢 SEO 100
🔴 PWA 33

Lighthouse ran on https://deploy-preview-2179--asyncapi-website.netlify.app/

@akshatnema
Copy link
Member

@derberg Your review is required!!

@captain-Akshay
Copy link
Contributor Author

@derberg ? it would be great if you could ?

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

Perfect job, sorry @captain-Akshay for keeping you waiting and 🙏🏼 for your patience

@derberg derberg changed the title feat: added docker file chore: added docker file Dec 19, 2023
@derberg derberg changed the title chore: added docker file chore: add docker file Dec 19, 2023
@derberg
Copy link
Member

derberg commented Dec 19, 2023

/rtm

@asyncapi-bot asyncapi-bot merged commit 70011d3 into asyncapi:master Dec 19, 2023
18 of 19 checks passed
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.

Addition of docker
4 participants