This diff is for review purposes only. In order to preserve history, I will need to land
these commits via git push rather than arc land.
Note the files will be located at /abcbot at the top level.
Details
Details
- Reviewers
deadalnix - Group Reviewers
Restricted Project
Build instructions in README
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- abcbot-commits
- Lint
Lint Errors Severity Location Code Message Error abcbot/abcbot.py:17 F401 flake8 F401 Error abcbot/abcbot_test.py:72 E131 flake8 E131 Error abcbot/abcbot_test.py:91 E131 flake8 E131 Error abcbot/abcbot_test.py:106 E131 flake8 E131 Error abcbot/abcbot_test.py:116 E131 flake8 E131 Error abcbot/abcbot_test.py:391 PYTHON_ENCODING1 Encoding should be specified when opening a text file. Error abcbot/server.py:8 F401 flake8 F401 Error abcbot/server.py:11 F401 flake8 F401 Error abcbot/server.py:19 F401 flake8 F401 Error abcbot/server.py:126 F841 flake8 F841 Error abcbot/slackbot_test.py:8 F401 flake8 F401 Error abcbot/teamcity_test.py:12 F401 flake8 F401 Error abcbot/test-request.sh: SHELL_LOCALE1 `export LC_ALL=C` or `export LC_ALL=C.UTF-8` should be the first statement. Auto-Fix abcbot/abcbot.py:1 CFMT Code style violation Auto-Fix abcbot/abcbot_test.py:1 CFMT Code style violation Auto-Fix abcbot/constants.py:1 CFMT Code style violation Auto-Fix abcbot/mocks.py:1 CFMT Code style violation Auto-Fix abcbot/phabricator_wrapper.py:1 CFMT Code style violation Auto-Fix abcbot/server.py:1 CFMT Code style violation Auto-Fix abcbot/slackbot.py:1 CFMT Code style violation Auto-Fix abcbot/slackbot_test.py:1 CFMT Code style violation Auto-Fix abcbot/teamcity.py:1 CFMT Code style violation Auto-Fix abcbot/teamcity_test.py:1 CFMT Code style violation Auto-Fix abcbot/testutil.py:1 CFMT Code style violation Advice abcbot/test-request.sh:5 SC2016 ShellCheck found an issue: - Unit
No Test Coverage - Build Status
Buildable 9221 Build 16387: Default Diff Build & Tests Build 16386: arc lint + arc unit
Event Timeline
Comment Actions
I don't think it makes sense to add a new folder for every tool out there. This dilutes the signal that something at the root is important.
This also contains a bunch of docker and other deployment stuff that really are not relevant to anyone, and depends on a setup that doesn't live in this repo to begin with. In general, it's a pretty bad idea to provide everybody with all the details of servers setup, but, in addition, it's not I can submit a patch that change the docker file and the bot will be shut down, and a new docker image will be produced with the new file, so that's pretty meaningless to have them here. I don't see it ever making sense either. This just create entanglement between systems that really don't need to be. Does this bot care that it runs in a docker container? If so, then that's pretty alarming in term of separation of concerns, but if not, then why is it packaged with a specific config there?
| abcbot/.template.env | ||
|---|---|---|
| 1 | ??? | |
Comment Actions
Fair. You weren't present at the last team meeting where we discussed this. I'm comfortable with it being present in the root or contrib.
This also contains a bunch of docker and other deployment stuff that really are not relevant to anyone, and depends on a setup that doesn't live in this repo to begin with. In general, it's a pretty bad idea to provide everybody with all the details of servers setup, but, in addition, it's not I can submit a patch that change the docker file and the bot will be shut down, and a new docker image will be produced with the new file, so that's pretty meaningless to have them here. I don't see it ever making sense either. This just create entanglement between systems that really don't need to be. Does this bot care that it runs in a docker container? If so, then that's pretty alarming in term of separation of concerns, but if not, then why is it packaged with a specific config there?
Ya, the current config is confusing because it shouldn't be packaged with it's own nginx.conf. This is something I'm already planning on separating out. If you would rather I do this before migrating it to the ABC repo, then I'll need more review support because the bitcoinabc.org github is basically dead. The benefit of migrating sooner rather than later is that I get more eyeballs on reviews and faster iteration speeds. The downside is introducing bad design decisions into this repo that are present in existing code in the other repo. This will last until it's fixed.
Besides the nginx stuff, the Dockerfile is very useful because we can push it to the registry and deploy it using whatever config we want. This is the model we're moving towards for deploying our dev services like abcbot as well. To answer your question, the site doesn't care that it's running in a docker container, but it does provide nice separation of concerns since the build-side of the docker image is handled by this repo and then deployments can happen elsewhere in a more controlled environment.