feat: add ckanext-file-uploader #233
Conversation
A-Souhei
dismissed
their stale review
{{ h.get_package_stats(package.id) }} is not found
| @@ -75,6 +75,7 @@ ckanext-scheming = {editable = true, ref = "8d62a28370c4f231dbf2f9688ede93a03564 | |||
| ckanext-versions = {editable = true, ref = "3a6094acd730f3495b0f8b99b14a2340042a721b", git = "https://github.com/datopian/ckanext-versions"} | |||
| ckanext-dcat = {editable = true, ref = "51d651315ff2ff594ebd6eb1d658bc7e9553310c", git = "https://github.com/ckan/ckanext-dcat"} | |||
| ckanext-short-urls = {editable = true, ref = "2.0.0", git = "https://github.com/fjelltopp/ckanext-short-urls"} | |||
| ckanext-file-uploader = {editable = true, ref = "1.0.0-rc2", git = "https://github.com/fjelltopp/ckanext-file-uploader"} | |||
There was a problem hiding this comment.
It's the second release candidate (because there was nothing on main to release); I can update it now we've merged.
(I probably should have kept this as draft whilst it was pointing to a release candidate but I forgot lol)
| # Build React components | ||
| cd /usr/lib/ckan/ckanext-zarr/ckanext/zarr/react/ && \ | ||
| npm install && \ | ||
| # Build fjelltopp file uploader js and css files |
There was a problem hiding this comment.
There's something slightly odd about this file to me, it's a little tricky to articulate it clearly. It looks like this is trying to be a general solution for all projects, but is found in a project-specific file, and then still needs to know the domain of a certain projects. So it's a sort of half way house being neither general or specific?
There was a problem hiding this comment.
Yes, so all I've done here is copy the pattern we already use for theme. I would love there to be a more generic solution - the ideal would be a way to run scripts on spin up from within a CKAN extension but I don't thiiiink that's possible is it?
| if [ -d "$FILE_UPLOADER" ]; then | ||
| cd "$FILE_UPLOADER/react" | ||
| # nvm use | ||
| if [ "$CKAN_SITE_URL" = "http://zarr.minikube" ]; then |
There was a problem hiding this comment.
Presumably the purpose of this line is to determine fi you are in the local env or not?
Sometimes I really wish the CKAN pod just had an env var that told us which environment we were in. There's been a few occasions I wanted this. It really shouldn't be hard right - just print the contents of an ansible var into an ENV VAR in the ckan.yaml kubernetes template? I feel sure I raised this before and was told there was a reason why this was a bad idea.
Anyhow, this line confused me for quite a while and looked like a code sniff as it makes this code block project-specific whereas otherwise it would be general across all projects. Can we at least put in an explanatory comment?
| FILE_UPLOADER="$CKAN_VENV/src/ckanext-file-uploader" | ||
| if [ -d "$FILE_UPLOADER" ]; then |
There was a problem hiding this comment.
So it's these lines that make the file feel like it is written to be general across all projects. Are they needed? or will we only copy and paste this code block into projects where ckanext-file-uploader is installed?
There was a problem hiding this comment.
I think I agree with your thoughts on this - maybe we can brainstorm a better solution.
@A-Souhei - this is the Google Analytics issue that I raised with you so you're the one fixing this 🤣 |
|
@jonathansberry - is the Plugin Observer interface the right way to do this initialisation work in a general fashion? https://docs.ckan.org/en/2.11/extensions/plugin-interfaces.html#ckan.plugins.interfaces.IPluginObserver |
|
@ChasNelson1990 |
Description
Removes all the previous file uploader JS, CSS and templates and instead install v1.0.0 of the new ckanext-file-uploader.
relates fjelltopp/ckanext-file-uploader#2
relates fjelltopp/fjelltopp-infrastructure#343
Dependency Changes
Added
ckanext-file-uploader==v1.0.0-rc1to Pipfile and locked.Testing
Have run zarr-ckan locally with these changes and everything is working as expected.
Environment Changes
Have updated
bootstrap.shbased on the pattern @A-Souhei introduced with fjelltopp-theme.Checklist