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

Use CMD instead of ENTRYPOINT #77

Closed
wants to merge 3 commits into from

Conversation

davidbarratt
Copy link
Contributor

@davidbarratt davidbarratt commented May 30, 2021

Fixes #76

shamaevnn
shamaevnn previously approved these changes Jun 1, 2021
@justinengland
Copy link
Member

branch has conflicts. also, not 100% sure I agree with the idea that you must be able to run esoteric commands. Docker exec can drop you to shell on the container.

@davidbarratt
Copy link
Contributor Author

branch has conflicts. also, not 100% sure I agree with the idea that you must be able to run esoteric commands. Docker exec can drop you to shell on the container.

I'll fix the conflicts. docker exec only works if it's currently running. What if I want to generate plots instead of startup a farmer? Should I just be able to execute the relevant chia command?

@justinengland
Copy link
Member

i would say that plotting from the container is at best a secondary concern. Not really a use case Id like to bake into the default setup.

its easy enough imo to get into the container run time to start plotting.

its a fair point. let me think through the orchestration changes I would need to make for GHA.

@skweee skweee mentioned this pull request Jun 1, 2021
@github-actions
Copy link

'This PR has been flagged as stale due to no activity for over 60
days. It will not be automatically closed, but it has been given
a stale-pr label and should be manually reviewed.'

Copy link
Contributor

@cmmarslender cmmarslender left a comment

Choose a reason for hiding this comment

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

Looking through PRs today, and I think I do like how the accomplishes your goal of supporting chia commands without necessarily having chia running - Its a bit out of date with main, but if you can get it updated with latest changes from main and take a look at the proposed change below (and the associated problem with that tail) I can do some testing on the container this week to make sure it still works for all the existing cases.

chia start farmer
fi

tail -f ~/.chia/mainnet/log/debug.log
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tail -f ~/.chia/mainnet/log/debug.log
tail -f $CHIA_ROOT/log/debug.log

This file doesn't necessarily exist right away either - I just tested a build of this locally (with latest main merged in) and I'm getting:

...
Daemon not started yet
Starting daemon
chia_harvester: started
chia_farmer: started
chia_full_node: started
chia_wallet: started
tail: cannot open '/root/.chia/mainnet/log/debug.log' for reading: No such file or directory
tail: no files remaining

Need to either go back to the previous while true; ... stuff, or else, maybe just touch $CHIA_ROOT/log/debug.log before trying to tail it

@cmmarslender
Copy link
Contributor

Closing in favor of #110 (includes these changes plus a few fixes and resolving conflicts)

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.

ENTRYPOINT should be CMD
4 participants