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

Docker file for CSSIFY #23

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Docker file for CSSIFY #23

wants to merge 6 commits into from

Conversation

aeyshubh
Copy link

@aeyshubh aeyshubh commented Feb 26, 2021

issue #5

Description

I have created a docker file for CSSIFY with NOde version of 14 which is compatible for the project.

Motivation and Context

How Has This Been Tested?

The file was successfully built and no errors were generated.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@aeyshubh aeyshubh marked this pull request as ready for review February 26, 2021 04:00
Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

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

@ShubhamPatel33 , please do not commit the package-lock.json from the commit.

And can you show the POC maybe like a gif that the dockerfile is working?

@sansyrox
Copy link
Member

@ShubhamPatel33 , i was referring to this one: client/package-lock.json

@aeyshubh
Copy link
Author

Okay,is it fine now, only gif remaining,correct ?

@sansyrox
Copy link
Member

Yes, @ShubhamPatel33 . Even a simple video would suffice.

@aeyshubh
Copy link
Author

Created a small video how one can start the docker and have fun with it .

Uploading demo_dockerfile.mp4…

@aeyshubh
Copy link
Author

demo_dockerfile.mp4

@sansyrox
Copy link
Member

@ShubhamPatel33 , good work! But this is not what I was looking for. By deployment, we are looking to expose the port locally.

You'd have to do more work in this PR and create a way so that you can read access the port through your localhost.

@aeyshubh
Copy link
Author

@aeyshubh
Copy link
Author

@ShubhamPatel33 , good work! But this is not what I was looking for. By deployment, we are looking to expose the port locally.

You'd have to do more work in this PR and create a way so that you can read access the port through your localhost.

Thanks

@aeyshubh
Copy link
Author

aeyshubh commented Mar 1, 2021

Updating the video by the docker build and run command.

docker_final.mp4

@aeyshubh aeyshubh requested a review from sansyrox March 2, 2021 12:44
@sansyrox
Copy link
Member

sansyrox commented Mar 2, 2021

Looks good!

@@ -13,7 +13,7 @@
"react": "^16.13.1",
"react-bootstrap": "^1.0.1",
"react-dom": "^16.13.1",
"react-scripts": "^3.4.4",
"react-scripts": "^4.0.3",
Copy link
Member

Choose a reason for hiding this comment

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

why this change tho?

Copy link
Author

Choose a reason for hiding this comment

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

These changes were automatically been made while making the docker file

@@ -25,6 +25,7 @@
"helmet": "^3.22.0",
"node-sass": "^4.14.1",
"node-sass-json-importer": "^4.3.0",
"socket.io": "^3.1.2",
Copy link
Member

Choose a reason for hiding this comment

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

why did you add this module?

Copy link
Author

Choose a reason for hiding this comment

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

These changes were automatically been made while making the docker file

Copy link
Member

Choose a reason for hiding this comment

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

@ShubhamPatel33, please remove the socket io dependency as it is not required. And sorry for the slow reviews, have been a little occupied lately.

Copy link
Author

Choose a reason for hiding this comment

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

Removed the socket.io,will it gonna count in Mwoc as the PR was made in the mentioned date and the changes took some time .

@sansyrox
Copy link
Member

sansyrox commented Mar 2, 2021

All looks good, please do these minor tweaks before we merge it.

@aeyshubh
Copy link
Author

aeyshubh commented Mar 2, 2021

All looks good, please do these minor tweaks before we merge it.

Thanks, finally it will be merged .

@aeyshubh aeyshubh requested a review from sansyrox March 2, 2021 13:58
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