Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Include healthcheck logic for helper scripts running as sidecars #1842
base: alpha
Are you sure you want to change the base?
Include healthcheck logic for helper scripts running as sidecars #1842
Changes from 8 commits
0b27c4e
257647c
0644d5f
859b0e5
f752b1c
74aa459
1eda07b
a7d9a18
bd6d3dd
443eece
29aa4ce
505487a
338639c
cbdd386
36d6525
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
For docker/podman, there arent current instructions - we need to possibly attach a sample reference compose file , wherein PGPASSFILE can be added (or instructions added to be part of priv folder)
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.
Is the current Guild Ops DBSync documentation which mentions this considered insufficient in this case? It documents it at the top of the page, and lower down shows examples of using the priv folder as the path.
Also, this healtcheck won't impose any new requirements for using
dbsync.sh
in a container that don't already exist using it on bare metal.Line 120 comes from line 42 of
set_defaults()
function:guild-operators/scripts/cnode-helper-scripts/dbsync.sh
Lines 38 to 47 in 349dffd
Lines 121-122 come from lines 52-53 of
check_defaults()
function:guild-operators/scripts/cnode-helper-scripts/dbsync.sh
Lines 49 to 60 in 349dffd
A compose file example is definitely an option. However, I find it can also be a crutch by allowing users to skip reading documentation. If you consider it a "must have" then I think it should be part of another PR and have its own separate testing and review.
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 look at (atleast a big % of) docker users as spoon-fed users - not intended in any demeaning way, but the primary requirement of them using docker [besides virtual env isolation] would be simplicity - as we're not talking about slightly more experienced/advanced users already taking to next step and deploying in professional environments , their expectations will be not having to go through document pages beyond docker reference ones.
If someone running a manual setup is moving to docker, I dont expect them to have as many queries. It does kinda begs the question if docker users would also share the pre-reqs on home page - in my experience of reading github issues on IO/intersectmbo issues, a very large section does not.
Ye - I agree. That - if added - does not need to be part of this PR indeed
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.
Talking about dbsync in particular, I would think this would likely be a seperate docker image altogether (which is why I mentioned earlier there is some work to be done):
In theory , same can be done for smaller components, but there is little value as their setups are mostly the same. For instance, for something like ogmios - we dont need a custom image, official image is perfectly usable.
Once above is available, a [docker|podman]-compose will essentially just give a reference on sharing volumes between the pods|containers
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.
Sounds good, after this, and some other documentation issues/PR are resolved I'll open one and start a compose example.
Nice. So far I've been running the upstream db-sync image, as well as ogmios, and guild ops for node, cncli, and mithril. I'm hereby offering to assist with a new container if any help is needed.
Yep. The socket, configs (files) and just about every other subdir of CNODE_HOME lend themselves to volume usage. Not scripts though. The excessive work trying to make scripts a shared volume wasn't worth the time I initially spent to get it working. I've found a container environment file with all vars + a completely stock scripts directory and env file inside the container to be the simplest way to run separate containers per process with shared vars.