-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: add docker configuration for the simulator #416
feat: add docker configuration for the simulator #416
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe changes must be made to match the agreed upon team convention for making gradle tasks.
72a5eb0
to
453f534
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple minor (non-blocking) items.
Signed-off-by: georgi-l95 <[email protected]> improve dockerfile Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]> cleanup docker setup Signed-off-by: georgi-l95 <[email protected]> rework documentation Signed-off-by: georgi-l95 <[email protected]> spotless Signed-off-by: georgi-l95 <[email protected]> fix conflicts Signed-off-by: georgi-l95 <[email protected]> address feedback Signed-off-by: georgi-l95 <[email protected]> revert documentation improvements Signed-off-by: georgi-l95 <[email protected]> revert docs Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]> remove docker compose version Signed-off-by: georgi-l95 <[email protected]> add update-env script Signed-off-by: georgi-l95 <[email protected]> rename file Signed-off-by: georgi-l95 <[email protected]> simplify docs Signed-off-by: georgi-l95 <[email protected]> update order Signed-off-by: georgi-l95 <[email protected]>
1ca124a
to
c391d57
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much improved @georgi-l95.
A few more things I think we should do, described below, but here the gist of it:
- Add
startDockerContainerDebug
andstopDockerContainer
- We can remove the
prepare-docker.sh
and simplify a lot the process - The copying of resources in the
prepare-docker.sh
must be extracted and done in the copy task in gradle which you created. - We can very easily add some conditional logic to the
update-env.sh
to enable debugging for example.
The inspiration is coming from server.
Signed-off-by: georgi-l95 <[email protected]>
Signed-off-by: georgi-l95 <[email protected]>
Quick note. Consensus teams were looking to use the simulator for their own testing, and found the readme and quickstart woefully outdated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@georgi-l95 this looks amazing!
However I would suggest to revert the createDotEnv
chagnes and simply call the ./update-env.sh
directly at the startDockerContainer
task. This simplifies things a lot and we do not touch things that are planned to be removed because were a temporary workaround.
Signed-off-by: georgi-l95 <[email protected]>
Yes, I've noticed that too, mostly about configuration and opened a issue couple of days ago. #407 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@georgi-l95 looks great!
Signed-off-by: georgi-l95 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 🔥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #416 +/- ##
=========================================
Coverage 96.77% 96.77%
Complexity 409 409
=========================================
Files 82 82
Lines 1458 1458
Branches 90 90
=========================================
Hits 1411 1411
Misses 34 34
Partials 13 13 |
Description:
This PR simplifies the simulator development process by introducing Docker containerization and improves documentation clarity.
The changes streamline the development process by replacing manual configuration steps with automated Docker container setup, making it easier for users to run and monitor the simulator in both Publisher and Consumer modes.
Related issue(s):
Fixes #412
Notes for reviewer:
Checklist