-
Notifications
You must be signed in to change notification settings - Fork 26
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
Allow renaming datasets & dataset with duplicate names #8075
Conversation
…ir dataset addressing scheme
… the check for already existing name)
…gaId - Includes moving ObjectId to uitls package
…store & add legacy routes - Undo renaming DataSourceId to LegacyDataSourceId
…ow-dataset-renaming
…d required legacy adaption
- no legacy routes needed as not used by wklibs
I now removed the
I'll do testing of the new legacy routes later |
Looks right to me! |
…ow-dataset-renaming
- also remove unised injections and imports
Hi @fm3, I added the required legacy routes to remove For the curl commands, you first need to fill in the id cookie before you can run the scripts. Before testing here is one more important thing to double check: The legacy routes now include both: Checklist for testing new Legacy Routes
Testing create & update task -> :shurg: I did this manually in firefox with the edit & resent feature on the dev instance. In case you want to test more complicated routes:curl command for checking task creation (setting id cookie is required)curl 'https://allowdatasetrenaming.webknossos.xyz/api/tasks' \
-H 'accept: application/json' \
-H 'accept-language: en-US,en;q=0.9,de;q=0.8' \
-H 'cache-control: no-cache' \
-H 'content-type: application/json' \
-H 'cookie: id=<fill-me-in>' \
-H 'origin: https://allowdatasetrenaming.webknossos.xyz' \
-H 'pragma: no-cache' \
-H 'priority: u=1, i' \
-H 'referer: https://allowdatasetrenaming.webknossos.xyz/tasks/create' \
-H 'sec-ch-ua: "Google Chrome";v="131", "Chromium";v="131", "Not_A Brand";v="24"' \
-H 'sec-ch-ua-mobile: ?0' \
-H 'sec-ch-ua-platform: "Linux"' \
-H 'sec-fetch-dest: empty' \
-H 'sec-fetch-mode: cors' \
-H 'sec-fetch-site: same-origin' \
-H 'user-agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/131.0.0.0 Safari/537.36' \
--data-raw '[{"taskTypeId":"63721e2cef0100470266c485","neededExperience":{"domain":"sampleExp","value":1},"pendingInstances":1,"projectName":"sampleProject","boundingBox":null,"dataSet":"kiwi","editPosition":[0,0,0],"editRotation":[0,0,0],"baseAnnotation":null}]' Test task update: (setting id cookie is required)curl 'https://allowdatasetrenaming.webknossos.xyz/api/tasks/6746ecc6010000d80015ea8c' \
-X 'PUT' \
-H 'accept: application/json' \
-H 'accept-language: en-US,en;q=0.9,de;q=0.8' \
-H 'cache-control: no-cache' \
-H 'content-type: application/json' \
-H 'cookie: id=<fill-me-in>' \
-H 'origin: https://allowdatasetrenaming.webknossos.xyz' \
-H 'pragma: no-cache' \
-H 'priority: u=1, i' \
-H 'referer: https://allowdatasetrenaming.webknossos.xyz/tasks/6746ecc6010000d80015ea8c/edit' \
-H 'sec-ch-ua: "Google Chrome";v="131", "Chromium";v="131", "Not_A Brand";v="24"' \
-H 'sec-ch-ua-mobile: ?0' \
-H 'sec-ch-ua-platform: "Linux"' \
-H 'sec-fetch-dest: empty' \
-H 'sec-fetch-mode: cors' \
-H 'sec-fetch-site: same-origin' \
-H 'user-agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/131.0.0.0 Safari/537.36' \
--data-raw '{"taskTypeId":"63721e2cef0100470266c485","neededExperience":{"domain":"sampleExp","value":1},"pendingInstances":2,"projectName":"sampleProject","boundingBox":null,"datasetId":"kiwi","editPosition":[0,0,0],"editRotation":[0,0,0]}' |
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.
LGTM & Works for me =) Thanks! I added two more small comments. Also, have a look at the merge conflicts. None of that should be much work. Let’s ship it!
sil: Silhouette[WkEnv])(implicit ec: ExecutionContext, bodyParsers: PlayBodyParsers) | ||
extends Controller { | ||
|
||
/* to provide v8, remove legacy routes */ |
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.
Not sure I understand this comment. What legacy routes are removed? (I mean, the PR removes some, but this comment appears to be about the following code lines). Maybe it should be something like “to provide v8, add dataSet to task json?” Or maybe remove these comments altogether.
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.
Ah damn it. This is a mixture of a todo comment which I forgot to remove and copy & paste mistake.
I renamed all such occurrences to simple /* provide vX */
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.
Or maybe remove these comments altogether.
Something like these comments were present previously (I think). Thus, I kept them to "order" the code a little :)
I can also remove them in case you prefer that :)
CHANGELOG.unreleased.md
Outdated
@@ -26,6 +27,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released | |||
- Fix a bug where dataset uploads would fail if the organization directory on disk is missing. [#8230](https://github.com/scalableminds/webknossos/pull/8230) | |||
|
|||
### Removed | |||
- Removed legacy routes for versions 2,3 and 4. [#8075](https://github.com/scalableminds/webknossos/pull/8075) |
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.
- Removed legacy routes for versions 2,3 and 4. [#8075](https://github.com/scalableminds/webknossos/pull/8075) | |
- Removed support for HTTP API versions 3 and 4. [#8075](https://github.com/scalableminds/webknossos/pull/8075) |
(v2 was already removed previously) Could you also mention this in the migration guide please? (If someone has old code interacting with the HTTP API, they need to update)
…ow-dataset-renaming
- fix changelog entry - add migration entry about dropped api versions
merge conflicts & your two comments are done now. So it's ready for 🚢 |
🎉 🤞 |
Further Notes:
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
organization_name
in worker toorganization_id
. see Rename organization_name to organization_id in worker args #8038LinkedLayerIdentifier
still uses the datasetName as an identifier[ ] the datasetThe dataset seems to be broken. Could reproduce this on other branchesC555_tps_demo
has quite some bucket loading errors. Unsure why some buckets do not workDatasetURIParser
Issues:
(Please delete unneeded items, merge only when none are left open)