Project

General

Profile

Actions

Feature #21717

closed

Add CORS headers to keepstore GET/HEAD responses

Added by Tom Clegg 10 days ago. Updated 1 day ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Keep
Story points:
-

Subtasks 1 (0 open1 closed)

Task #21728: Review 21717-keepstore-corsResolvedBrett Smith04/26/2024Actions
Actions #1

Updated by Tom Clegg 10 days ago

  • Status changed from New to In Progress
Actions #2

Updated by Peter Amstutz 9 days ago

  • Target version changed from Development 2024-04-24 sprint to Development 2024-05-08 sprint
Actions #3

Updated by Peter Amstutz 9 days ago

  • Category set to Keep
Actions #5

Updated by Tom Clegg 8 days ago

  • All agreed upon points are implemented / addressed.
    • also support pre-flight requests for PUT (write block), POST (update trash lists, etc), because why not
  • Anything not implemented (discovered or discussed during work) has a follow-up story.
    • TBD: if we want to release this with a future 2.x, we'll need a separate branch -- this won't cherry-pick across the #2960 keepstore refactor.
  • Code is tested and passing, both automated and manual, what manual testing was done is described
    • automated tests only.
  • Documentation has been updated.
    • N/A. Our docs treat keepstore interfaces as an internal implementation detail. Or is there somewhere this should be noted?
  • Behaves appropriately at the intended scale (describe intended scale).
    • N/A
  • Considered backwards and forwards compatibility issues between client and server.
    • N/A
  • Follows our coding standards and GUI style guidelines.
Actions #6

Updated by Brett Smith 8 days ago

Looks good to me, but question about this:

-       replicas, _ := strconv.Atoi(req.Header.Get("X-Arvados-Replicas-Desired"))
+       replicas, _ := strconv.Atoi(req.Header.Get("X-Arvados-Desired-Replicas"))

Am I right this is an unrelated bugfix? That's totally fine, especially given how simple it is, the reason I'm flagging it is because we tend to overlook stuff like this in the release notes. It would be nice to discuss and figure out a way to prevent that.

Thanks.

Actions #7

Updated by Tom Clegg 3 days ago

Brett Smith wrote in #note-6:

Am I right this is an unrelated bugfix?

Yes. The bug was introduced in #2960 and hasn't been in any released version.

But the best part is that it's a broken fix. The real header is X-Keep-... not X-Arvados-... ... which is why I didn't find it when changing the other occurrences of "X-Keep-..." to keepclient.XKeep... consts.

Fixed:

21717-keepstore-cors @ 293631794d64c64986ba0db2568345c005c90790 -- developer-run-tests: #4196

Actions #8

Updated by Tom Clegg 1 day ago

  • Status changed from In Progress to Resolved
Actions

Also available in: Atom PDF