-
Notifications
You must be signed in to change notification settings - Fork 24
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
Voxelytics routes #6416
Voxelytics routes #6416
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good already! :) I added a few notes. I think the overall structure works well. One thing that differs from a lot of the rest of wk is the combined DAO with complex queries, but I think it works well for this use case.
This kind of makes the standard access query mechanism a bad fit, though. I see that you have solved auth in a different way, and I’d say that’s fine. You could also try to bend the existing mechanism for this (inherit from SecuredSQLDAO, overwrite collectionName and see what it gets you), but I think since there are so many different tables, this does not really work well. Happy to talk about this in person again in the coming days :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback! I went all your comments.
I merged in the objectid-json
branch, because it helps a lot! But it adds some unrelated changes. Will clean that up once #6413 is merged.
@@ -82,6 +82,7 @@ features { | |||
allowDeleteDatasets = true | |||
# to enable jobs for local development, use "yarn enable-jobs" to also activate it in the database | |||
jobsEnabled = false | |||
voxelyticsEnabled = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be enabled by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that's just for development.
voxelytics { | ||
staleTimeout = 20 minutes | ||
elasticsearch { | ||
uri = "http://localhost:9200" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my guess is the uri should be empty in the repository, and only added for the individual deployments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is moving in a very good direction :) You have been busy! I added a few more comments to the backend code. Looking forward to testing with the frontend next.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backend LGTM and seems to work :) I will admit I did not test every code path, and we will likely find more things as this lives on, but I’d say for a first version this is ready for some dogfooding.
Please add a changelog entry and migration guide, remove the for-development-only config and some debug logging (I think), then when the frontend is approved too this should be good to go :)
Co-authored-by: Florian M <[email protected]>
* wip voxelytics workflow viewer frontend * fixes JS code * stuff * styling * expose features.voxelyticsEnabled * revert changes to Application.scala * format * pr feedback * output ES2020 * styling * styling Co-authored-by: Florian M <[email protected]>
Adds the backend code for the Voxelytics workflow viewer to webKnossos.
Steps to test:
(Please delete unneeded items, merge only when none are left open)