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

2.8.x: Qld Government core updates: Incorporate delete() and download() into IUploader and ResourceUpload for better cloud integrations #5766

Closed
wants to merge 5 commits into from

Conversation

duttonw
Copy link

@duttonw duttonw commented Nov 30, 2020

This is the work that has been completed by Qld Government in maintaining https://www.data.qld.gov.au and https://www.publications.qld.gov.au.

This includes a change to the IUploader interface which allows better integrations for archiver plugin or similar in verifying files with AWS s3 plugin for resource files off disk. (Allowing nearly immutable web front of house instances for high scaling without the need of a network file store for uploads)

This is backwards compatible, if a plugin extends IUploader but does not incorporate download, then it will fallback to previous functionality.

Proposed fixes:

Incorporate delete() and download() into IUploader, update ResourceUpload, Upload with functionality from controller resource_download to allow better cloud integrations

  • Add metadata to IUploader interface's
  • Add new resource_file_metadata_show get api to surface metadata checks, usage case is archiver or similar verifying files have not change or disappeared
  • Hook in delete uploader in resource delete action logic, use upload download logic fully
  • Remove unneeded code as its now in the ResourceUpload class, call upload delete when a resource is also deleted
  • Uploader and interface update so signature has filename
  • Allow filenames to be preserved in uploader
  • Add logging if delete method not found
  • Need package dict to call uploader correctly
  • Add metadata to uploader interface, next step, surface it via package…

Other Fixes

  • Ignore auth check on loading user details during password resets
  • Reduce race condition during datastore via adding error handling for race condition during datastore file uploads, Error in datastore updating resource #3980
  • Stop anyone from editing usernames
    • as sysadmins can already do anything so allow sysadmins to edit usernames
    • add nose test to ensure that sysadmins can change account names
    • adjust test password handling to match other tests
    • redirect after updating username
    • include extra environment when redirecting
    • use 'submit_and_follow' helper to simplify proper redirection
    • adjust test steps to better match successful user edit test
    • update context username after updating it in the account
  • Replace freshness-based check with a flag
    • don't update the package modification timestamp if we're only updating the resources
    • don't update package modification timestamp more than once every ten seconds, this should reduce lock contention on resource updates

Features:

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

Please [X] all the boxes above that apply

…sernames (login cookie forgery)

- as sysadmins can already do anything so allow sysadmins to edit usernames
- add nose test to ensure that sysadmins can change account names
- adjust test password handling to match other tests
- redirect after updating username
- include extra environment when redirecting
- use 'submit_and_follow' helper to simplify proper redirection
- adjust test steps to better match successful user edit test
- update context username after updating it in the account
- don't update the package modification timestamp if we're only updating the resources
- don't update package modification timestamp more than once every ten seconds, this should reduce lock contention on resource updates
- add comment to explain the delay on updating modification timestamp,
@duttonw duttonw changed the title Qgov master 2.8.6 single Qld Government core updates: Incorporate delete() and download() into IUploader and ResourceUpload for better cloud integrations Nov 30, 2020
@duttonw duttonw changed the title Qld Government core updates: Incorporate delete() and download() into IUploader and ResourceUpload for better cloud integrations 2.8.x: Qld Government core updates: Incorporate delete() and download() into IUploader and ResourceUpload for better cloud integrations Nov 30, 2020
…load, Upload with functionality from controller resource_download to allow better cloud integrations

- Add metadata to IUploader interface's
- Add new resource_file_metadata_show get api to surface metadata checks, usage case is archiver or similar verifying files have not change or disappeared
- Hook in delete uploader in resource delete action logic, use upload download logic fully
- Remove unneeded code as its now in the ResourceUpload class, call upload delete when a resource is also deleted
- Uploader and interface update so signature has filename
- Allow filenames to be preserved in uploader
- Add logging if delete method not found
- Need package dict to call uploader correctly
- Add metadata to uploader interface, next step, surface it via package…
@Zharktas
Copy link
Member

This PR should be done against master branch as in practice no new interfaces are added to old releases.

@duttonw
Copy link
Author

duttonw commented Nov 30, 2020

Thanks for the tip @Zharktas , We are on 2.8.x atm so I'm working towards seeing how well cherry-picking these commits to master goes.

@ThrawnCA
Copy link
Contributor

@Zharktas Note that some of these are bug fixes, like ignoring auth checks during password resets (which becomes necessary if you set ckan.auth.public_user_details = false).

@amercader
Copy link
Member

@duttonw, @ThrawnCA Thanks for this. If individual changes can be split into separate PRs (specially bug fixes) that makes reviewing and backporting much easier for us

@duttonw
Copy link
Author

duttonw commented Dec 10, 2020

@duttonw, @ThrawnCA Thanks for this. If individual changes can be split into separate PRs (specially bug fixes) that makes reviewing and backporting much easier for us

Will do @amercader
I've been have (fun/not fun) time accomodating to the new test system on the master branch for getting this work ready to be incorporated there. What is the usual developer environment you use?

@duttonw
Copy link
Author

duttonw commented Dec 11, 2020

part 1 of breakout pr's #5791

@pdelboca
Copy link
Member

Closing due to inactivity and as we do not support CKAN 2.8 anymore.

Thanks for submitting!

@pdelboca pdelboca closed this Apr 14, 2023
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.

None yet

5 participants