Skip to content

feat: changed container work dir from app to opt #872

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AlexKehayov
Copy link
Contributor

Reviewer Notes

Replaced /app with /opt in the server's Docker and Helm chart configurations, as well as in the simulator's Dockerfile.
Tested local run and Docker run for the server and the simulator.
Tested K8 run for the server.

P.S.
I was unable to test the simulator in K8 due to difficulties connecting it to a running BN server, likely because of a misunderstanding of the configuration steps on my part. While the simulator runs fine in a Docker container and chart changes were not applied, it would still be good to be tested.

A thorough review of these changes, especially regarding the K8 integration, would be greatly appreciated.

Related Issue(s)

Closes #684

@AlexKehayov AlexKehayov marked this pull request as ready for review March 18, 2025 15:55
@AlexKehayov AlexKehayov requested review from a team as code owners March 18, 2025 15:55
@ata-nas ata-nas added this to the 0.8.0 milestone Mar 18, 2025
@ata-nas ata-nas added the pull request label to get past the "label required" check when no label is needed or appropriate. label Mar 18, 2025
@AlexKehayov AlexKehayov force-pushed the 684-container-work-dir-from-app-to-opt branch from 6becdd1 to a8d6815 Compare March 19, 2025 14:07
@AlexKehayov AlexKehayov force-pushed the 684-container-work-dir-from-app-to-opt branch 2 times, most recently from 955c213 to 0ee82c3 Compare March 19, 2025 15:25
@AlexKehayov
Copy link
Contributor Author

@jsync-swirlds @Nana-EC Thank you for the guidance and the reviews. Your help is much appreciated.

I am still trying to complete the testing of the Helm chart changes, but I am having some difficulties running the pods with local Docker images (containing the Dockerfile changes) instead of the ones from the registry. Perhaps someone more proficient with Kubernetes than I am could check whether the changes are valid or if a new issue related to the K8s part should be opened.

@jsync-swirlds
Copy link
Contributor

@jsync-swirlds @Nana-EC Thank you for the guidance and the reviews. Your help is much appreciated.

I am still trying to complete the testing of the Helm chart changes, but I am having some difficulties running the pods with local Docker images (containing the Dockerfile changes) instead of the ones from the registry. Perhaps someone more proficient with Kubernetes than I am could check whether the changes are valid or if a new issue related to the K8s part should be opened.

Perhaps @AlfredoG87 could take a look once the 0.6.1 release is done.

@AlexKehayov
Copy link
Contributor Author

AlexKehayov commented Mar 20, 2025

I managed to do the K8 run with the updated server Dockerfile locally. Attaching the results below.

image image image image

Copy link
Contributor

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

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

A few PRs (this included) will likely be delayed and impacted by significant ongoing rewrites of the code base.
I'm blocking the PR for now just to avoid any unintended conflicts and to save you effort.

Once things are in the clear we can decide whether to rebase (if all changes are still applicable), pull some changes into a new PR (in a subset is still applicable) or if to close this underlying ticket if it's no longer applicable.

Thanks for your interest and contributions here, we'll be laying out more tickets that the community can look to to pick up and also exploring how to have community calls to improve the efficiency and sharing of information.

@AlfredoG87 AlfredoG87 modified the milestones: 0.8.0, 0.9.0 Apr 3, 2025
@AlfredoG87 AlfredoG87 modified the milestones: 0.9.0, 0.10.0 Apr 17, 2025
@Nana-EC
Copy link
Contributor

Nana-EC commented Apr 28, 2025

Hey @AlexKehayov , feel free to rebase and update if you're able to now that the refactor to main has completed..
A lot has changed so it's possible the issue is no longer applicable or a larger or smaller effort than before.
Take your time and let us know what you find.

@AlexKehayov AlexKehayov force-pushed the 684-container-work-dir-from-app-to-opt branch from 0ee82c3 to d1add33 Compare April 29, 2025 09:02
@AlexKehayov
Copy link
Contributor Author

@Nana-EC, the conflicts are resolved :)

Copy link

codecov bot commented May 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

@@            Coverage Diff            @@
##               main     #872   +/-   ##
=========================================
  Coverage     82.07%   82.07%           
  Complexity      858      858           
=========================================
  Files           102      102           
  Lines          3694     3694           
  Branches        380      380           
=========================================
  Hits           3032     3032           
  Misses          525      525           
  Partials        137      137           
Files with missing lines Coverage Δ Complexity Δ
...ode/blocks/files/historic/FilesHistoricConfig.java 100.00% <ø> (ø) 1.00 <0.00> (ø)
...ck/node/blocks/files/recent/FilesRecentConfig.java 100.00% <ø> (ø) 1.00 <0.00> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull request label to get past the "label required" check when no label is needed or appropriate.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change our container work dir from /app to /opt
5 participants