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

[cli][m] removes old paster cli commands #5129

Closed
wants to merge 17 commits into from
Closed

[cli][m] removes old paster cli commands #5129

wants to merge 17 commits into from

Conversation

EvgeniiaVak
Copy link
Contributor

@EvgeniiaVak EvgeniiaVak commented Dec 18, 2019

Fixes #5110

Note

The order of where to pass flags has changed:
before they were put at the end:

paster plugin=ckan user add admin password=12345678 email=admin@admin.org -c development.ini

now after ckan:

ckan -c development.ini user add admin password=12345678 email=admin@admin.org

Proposed fixes:

Features:

  • includes tests covering changes
  • includes updated documentation
  • includes user-visible changes
  • includes API changes
  • includes bugfix for possible backport

@EvgeniiaVak
Copy link
Contributor Author

I removed the classes, extension paster cli commands still work checked with:

paster --plugin=ckan datastore set-permissions -c development.ini

Also updated user add user (and checked that it works).

Still WIP:

  • investigating if click_config_option and paster_click_group are needed
  • tests failing

@amercader amercader self-assigned this Dec 19, 2019
@amercader
Copy link
Member

amercader commented Dec 19, 2019

Great work @EvgeniiaVak ! The tests are failing because the Circle CI setup uses the old paster commands to init the database. You'll need to update those to the new version: https://github.com/ckan/ckan/blob/master/.circleci/config.yml#L61:L62

@tino097 @pdelboca this PR includes the changes in #5114 so perhaps we can close that one.
@EvgeniiaVak does the ckan sysadmin add command work fine for you in your PR?

@EvgeniiaVak
Copy link
Contributor Author

EvgeniiaVak commented Dec 19, 2019

@amercader

does the ckan sysadmin add command work fine for you in your PR?

yes, it works fine.

@EvgeniiaVak
Copy link
Contributor Author

@amercader
I fixed the settings you've mentioned at https://github.com/ckan/ckan/blob/master/.circleci/config.yml#L61:L62. Some tests still fail - because they were the cli tests, and there were some files importing old commands. I'm working on replacing those.

@amercader
Copy link
Member

@EvgeniiaVak do you need help to get this over the line? Looking at the failures they seem related to a file already gone from master.

@EvgeniiaVak
Copy link
Contributor Author

@amercader Ok, thanks, I'll update the branch from the master.

@EvgeniiaVak
Copy link
Contributor Author

@amercader
well, since all the deletions are already in master this issue #5110 can probably be closed, as it was about the deletions originally.

So this PR is now about adding the tests for ckan cli. I was trying to copy those from paster and remake them for click, and there is still one issue about zero exit code, when there probably shouldn't be one: #5158.

And also there are already new tests at ckan/tests/cli/test_cli.py, so what should I do with this PR:

  1. close it, no changes from it are actually needed
  2. ✔️add new tests to ckan/tests/cli/test_cli.py

ckan/tests/test_cli.py Outdated Show resolved Hide resolved
@amercader
Copy link
Member

@EvgeniiaVak if you could work on adding the missing tests on ckan/tests/cli/test_cli.py That would be great. You can use a new PR or this one, whatever is cleaner or easier for you.

EvgeniiaVak and others added 3 commits February 10, 2020 18:52
Co-Authored-By: Patricio Del Boca <patriciodelboca@gmail.com>
user add tests fail with sql exceptions
@EvgeniiaVak
Copy link
Contributor Author

EvgeniiaVak commented Feb 11, 2020

@amercader
reimplementing tests in the new way, I actually created separate files ckan/tests/cli/test_jobs.py and ckan/tests/cli/test_user.py (with the intention to delete ckan/tests/test_cli.py), but with the user tests I have some problems - there are SQL exceptions in the command output:

(sqlite3.OperationalError) no such table: user
[SQL: SELECT user.password AS user_password, user.id AS user_id, user.name AS user_name, user.fullname AS user_fullname, user.email AS user_email, user.apikey AS user_apikey, user.created AS user_created, user.reset_key AS user_reset_key, user.about AS user_about, user.activity_streams_email_notifications AS user_activity_streams_email_notifications, user.sysadmin AS user_sysadmin, user.state AS user_state 
FROM user 
WHERE user.name = ? OR user.id = ? ORDER BY user.name
 LIMIT ? OFFSET ?]
[parameters: ('test.ckan.net', 'test.ckan.net', 1, 0)]
(Background on this error at: http://sqlalche.me/e/e3q8)

And I found that ckan/tests/test_cli.py::TestUserAdd never actually worked.

maybe I need to update some settings for it to work?

@davidread
Copy link
Contributor

@EvgeniiaVak the tests run for me. Your error suggests you're using sqlite3, which was with the old test.ini. Try running them like this:

(default) vagrant@ubuntu-bionic:/usr/lib/ckan/default/src/ckan$ pytest --ckan-ini=test-core.ini ckan/tests/cli/
2020-02-25 20:20:02,527 CRITI [ckan.lib.uploader] Please specify a ckan.storage_path in your config
                         for your uploads
============================================================================== test session starts ===============================================================================
platform linux -- Python 3.6.9, pytest-4.6.5, py-1.8.1, pluggy-0.13.1
rootdir: /usr/lib/ckan/default/src/ckan, inifile: setup.cfg
plugins: split-tests-1.0.9, rerunfailures-8.0, pyfakefs-3.2, cov-2.7.1
collected 39 items

ckan/tests/cli/test_asset.py .                                                                                                                                             [  2%]
ckan/tests/cli/test_cli.py ..........                                                                                                                                      [ 28%]
ckan/tests/cli/test_config_tool.py .....                                                                                                                                   [ 41%]
ckan/tests/cli/test_jobs.py ................                                                                                                                               [ 82%]
ckan/tests/cli/test_search_index.py ..                                                                                                                                     [ 87%]
ckan/tests/cli/test_user.py .....                                                                                                                                          [100%]

=========================================================================== 39 passed in 36.53 seconds ===========================================================================

@davidread davidread mentioned this pull request Mar 6, 2020
5 tasks
@davidread
Copy link
Contributor

@EvgeniiaVak Thanks for all the work on this! I hope you don't mind but I've done a bit of tidying up and created a new PR: #5264

@davidread davidread closed this Mar 6, 2020
@EvgeniiaVak
Copy link
Contributor Author

Thank you @davidread!

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.

Remove old paster CLI commands
4 participants