search

stacks-network / stacks-blockchain-docker

Stacks-blockchain with API using docker compose
GNU General Public License v3.0
27 stars 37 forks source link

Feat/bitcoin flag newer format #74

Closed criadoperez closed 2 years ago

criadoperez commented 2 years ago

Adding Bitcoin node feature to the new format

criadoperez commented 2 years ago

@wileyj please review whenever you can. No user config files (yaml, toml or config) are ever modified during runtime for this feature.

wileyj commented 2 years ago

The Dockerfiles moved out of ./Dockerfiles dir to ./compose-files should be moved back - they're specifically meant to be separate since they're not docker compose files and can be built separately from docker-compose.

wileyj commented 2 years ago

functions setup_bitcoin_up and setup_bitcoin_down are identical. is that by design?

criadoperez commented 2 years ago

The Dockerfiles moved out of ./Dockerfiles dir to ./compose-files should be moved back - they're specifically meant to be separate since they're not docker compose files and can be built separately from docker-compose.

Yes and no.

I was kind of expecting this point to be an issue... :-)

Because the dockerfile has some variables it can never run by itself directly from docker alone. They only work if called with docker-compose from the script itself. When I have dockerfiles that are dependent of the dockercompose I prefer to have them inside the dockercompose folder.

However, I understand this is just a personal preference and both options work fine.

As you prefer the other logic, I'll move it back to the Dockerfiles folder in the next commit, no problem.

criadoperez commented 2 years ago

Converted to draft until all requested changes are completed. I will remove draft status once they are all resolved.

wileyj commented 2 years ago

The Dockerfiles moved out of ./Dockerfiles dir to ./compose-files should be moved back - they're specifically meant to be separate since they're not docker compose files and can be built separately from docker-compose.

Yes and no.

I was kind of expecting this point to be an issue... :-)

Because the dockerfile has some variables it can never run by itself directly from docker alone. They only work if called with docker-compose from the script itself. When I have dockerfiles that are dependent of the dockercompose I prefer to have them inside the dockercompose folder.

However, I understand this is just a personal preference and both options work fine.

As you prefer the other logic, I'll move it back to the Dockerfiles folder in the next commit, no problem.

Not quite - the only variable used is BTC_VERSION which is a build arg (default is set in the file, but can be overridden with a build arg of a different version. full disclosure - when i build local bitcoin docker images, this is the dockerfile i use so it does build correctly outside of compose

wileyj commented 2 years ago

The Dockerfiles moved out of ./Dockerfiles dir to ./compose-files should be moved back - they're specifically meant to be separate since they're not docker compose files and can be built separately from docker-compose.

Yes and no.

I was kind of expecting this point to be an issue... :-)

Because the dockerfile has some variables it can never run by itself directly from docker alone. They only work if called with docker-compose from the script itself. When I have dockerfiles that are dependent of the dockercompose I prefer to have them inside the dockercompose folder.

However, I understand this is just a personal preference and both options work fine.

As you prefer the other logic, I'll move it back to the Dockerfiles folder in the next commit, no problem.

Thanks - the overall idea of the last big merge i made was to simplify the directories. main goal was to have all the compose-files (and only the compose files) in this dir.

criadoperez commented 2 years ago

@wileyj, all your code reviews have been addressed. Thanks for your input.

criadoperez commented 2 years ago

Added functionality so bitcoin-core logs are also exported with ./manage.sh -n <network> -a logs export when bitcoin node is running. I noticed the nginx wasn't exporting the logs neither. Any reason why? I added it also, but if there was a reason to exclude it let me know.

criadoperez commented 2 years ago

New options when data is reset: 

$ sudo ./manage.sh -n testnet -a reset
[sudo] password for alejandro: 
Please confirm what persistent data you wish to delete: 

0. Cancel                 
1. Delete Persistent data for Stacks testnet only and leave Bitcoin blockchain data unaffected. 
2. Delete Persistent data for Stacks testnet and Bitcoin blockchain data in ./persistent-data/blockchain-bitcoin 
3. Delete Persistent data for Bitcoin blockchain data in ./persistent-data/blockchain-bitcoin only. 
Please note that BNS data will never get deleted. 

Type 0, 1, 2 or 3: 2

Ok. Delete Stacks and Bitcoin data. 
[ Success ]      Persistent data deleted for Bitcoin blockchain.
[ Success ]      Persistent data deleted for Stacks testnet.
criadoperez commented 2 years ago

shipit

Cool! I'll resolve the conflicts first and make the last changes we discussed before merging.