Ckan 211 #158
Ckan 211 #158
Conversation
chore(deps): update dependencies and settings for CKAN 2.11 compatibility
Reviewer's Guide by SourceryThis pull request upgrades CKAN to version 2.11.1, updates configurations for CKAN, supervisord, and docker-compose, and removes the vocabulary upload script. State diagram for CKAN container startup processstateDiagram-v2
[*] --> CheckDB: Start
CheckDB --> InitDB: DB Check
InitDB --> UpdatePlugins: DB Init
UpdatePlugins --> UpdateDatabase: Configure Plugins
UpdateDatabase --> InitHarvestDB: Update DB Schema
InitHarvestDB --> CheckSolr: Init Harvest DB
CheckSolr --> CreateAdmin: Solr Check
CreateAdmin --> Running: Create Sysadmin
Running --> [*]: Stop
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @hcvdwerf - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Avoid hardcoding the
POSTGRES_PASSWORDin the.envfile. (link) - Avoid hardcoding the
CKAN_DB_PASSWORDin the.envfile. (link) - Avoid hardcoding the
CKAN_SYSADMIN_PASSWORDin the.envfile. (link) - Avoid hardcoding the
CKAN_SMTP_PASSWORDin the.envfile. (link)
Overall Comments:
- The change from
db upgradetodb initin prerun.py is potentially dangerous -db initwill try to initialize a fresh database which could cause problems on existing installations. Please verify if this should bedb upgradeinstead.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🔴 Security: 4 blocking issues
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ckan/config/crontab
Outdated
| 0 5 * * * /usr/bin/ckan -c /srv/app/ckan.ini harvester clean-harvest-log | ||
| 0 5 * * * /usr/local/bin/ckan -c /srv/app/ckan.ini harvester clean-harvest-log |
There was a problem hiding this comment.
Cleaning logs every day might hinder any debugging when issues arise. Is there a need for this schedule, or can we move to a "once a week" schedule?
ckan/config/supervisord.conf
Outdated
| [supervisord] | ||
| ; Directory where Supervisor will store its state | ||
| logfile=/var/log/supervisor/supervisord.log | ||
| logfile_maxbytes=50MB | ||
| logfile_backups=10 | ||
| loglevel=info | ||
| pidfile=/var/run/supervisord.pid | ||
| nodaemon=false | ||
| minfds=1024 | ||
| minprocs=200 |
There was a problem hiding this comment.
Is this new config needed for the supervisor to function? If so, did the prev version not need it?
There was a problem hiding this comment.
Is necessary for specific supervisor configuration. Before it was part of docker image
ckan/Dockerfile.dev
Outdated
| RUN pip3 install -e git+https://github.com/CivityNL/[email protected]#egg=ckanext-scheming[requirements] | ||
|
|
There was a problem hiding this comment.
Running bin/compose build locally fails on this line. When changing to RUN pip3 install -e git+https://github.com/CivityNL/[email protected]#egg=ckanext-scheming[requirements] it does seem to work.
We need to investigate what the differences are between release-3.0.0-civity, release-3.0.0-civity-1, and release-3.0.0-civity-2.
There was a problem hiding this comment.
This URL seems to point to the branch release-3.0.0-civity, in which there were changes on January 8. If we pin this to commit e7a0b22, so git+https://github.com/CivityNL/ckanext-scheming.git@e7a0b22#egg=ckanext-scheming[requirements], the Docker image does build and also produces a running CKAN instance.
There was a problem hiding this comment.
I will open a new ticket by civity and will change it to release-3.0.0-civity-1 for now. Out of scope for CKAN 2.11 migration
|
Summary by Sourcery
Upgrade CKAN to version 2.11.1 and update the configuration for cron, supervisord, and the harvester.
Enhancements:
Build:
CI:
Tests: