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

Argument to Inlude Group/Org Users in Dump and Load (+ fixed tests) #208

Merged
merged 11 commits into from
Aug 25, 2023

Conversation

JVickery-TBS
Copy link
Contributor

feat(cli): added include-users option for group and org dump/load;

  • Added --include-users argument for group and org dump/load.

- Added `--include-users` argument for group and org dump/load.
- Changed ubuntu image from `20.04` to `latest`.
- Removed unsupported python version from `actions/setup-python@v4`.
- Added extra condition to catch possible key errors.
- Changed `python2` to just `python` in test class.
- Added new `include-users` arg to dump and load tests.
- Closed files from possible memory leaking.
@JVickery-TBS
Copy link
Contributor Author

Working on fixing the tests currently.

Python 2.7 support has been removed fully from actions/python-versions so we can no longer use that. I have also updated the base image of test workflow to use ubuntu-latest.

- Fixed issues with `test_upload` mock action for tests.
- Attempting to use python images as python 2 has been removed from the `actions/setup-python`.
@JVickery-TBS JVickery-TBS changed the title Argument to Inlude Group/Org Users in Dump and Load Argument to Inlude Group/Org Users in Dump and Load (+ fixed tests) Aug 23, 2023
@JVickery-TBS
Copy link
Contributor Author

Okay I also fixed the tests. Sadly python 2 is no longer supported in actions/setup-python but I was able to get around this by just using the docker python images for the respective versions.

There was also a reported possible memory leak when running the tests due to unclosed files from the dump and search subcommands when not using stdout. I have added a closing call to those files. Let me know if this is not necessary or bad to do.

@wardi
Copy link
Contributor

wardi commented Aug 24, 2023

IIUC include_users=True would only do something on sites where ckan.auth.public_user_details = False, and only when dumping groups/orgs, right? This change provides the option for the load command too which doesn't look like it will do anything.

@JVickery-TBS
Copy link
Contributor Author

@wardi Yeah, so it would really only be a thing for ckan.auth.public_user_details = False, which is False by default.

https://github.com/ckan/ckanapi/blob/master/ckanapi/cli/load.py#L267

def _copy_from_existing_for_update(obj, existing, thing):
    """
    modifies obj dict in place, copying values from existing.

    the id is alwasys copied from existing to make sure update updates
    the correct object.

    users lists for groups and orgs are maintained if not present in obj
    """
    if 'id' in existing:
        obj['id'] = existing['id']

    if thing in ('organizations', 'groups'):
        if 'users' not in obj and 'users' in existing:
            obj['users'] = existing['users']

There is code which copies over the users in the above function. So if the users are not available, it will use the ones from the load method (if the load is called with --include-users).

- Removed include users from dump subcommand and tests.
ckanapi/cli/dump.py Outdated Show resolved Hide resolved
@wardi wardi merged commit 4aa7d25 into ckan:master Aug 25, 2023
2 checks passed
@JVickery-TBS JVickery-TBS deleted the feature/group-org-dump-load-users branch September 18, 2023 14:31
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.

2 participants