Closed
Bug 1227606
Opened 9 years ago
Closed 7 years ago
hookGroupId and hookId id format limitations
Categories
(Taskcluster :: Services, defect)
Taskcluster
Services
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jonasfj, Assigned: dustin)
Details
We need to specify identifier limitations here:
https://github.com/taskcluster/taskcluster-hooks/blob/945f9d18c33094abbbbab7edfada8858715ab01b/routes/v1.js#L27
And in JSON schema..
Similar to how it's done here:
https://github.com/taskcluster/taskcluster-queue/blob/c82c5d0ba580e6389a97c748ed4b088e572f6117/routes/v1.js#L90-L97
I propose that we use:
var GENERIC_ID_PATTERN = /^[a-zA-Z0-9-_]{1,22}$/;
Maybe we can allow hookId to be all printable ascii chars. But having short
ids is useful for having them fit into pulse messages, task definitions, etc.
Of course they are slightly harder to scope.
---
If we don't we clearly need to specify exactly what they are. Maybe "all printable ascii chars" is okay. I'm just no a huge fan of / in the hookGroupId, though it would work as long as it's URI encoded which API clients will do automatically for URL parameters.
Assignee | ||
Comment 1•9 years ago
|
||
I'm OK with GENERIC_ID_PATTERN.
I did verify that slashes work, but it's true that they are ambiguous.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → alesilva241
Mentor: dustin
Assignee | ||
Comment 2•7 years ago
|
||
I think we should just limit this to printable ascii (same a scopes), but not limit its length. We don't currently include these identifiers in pulse routes, and I don't see why we would.
Jonas, concur?
Flags: needinfo?(jopsen)
Comment 3•7 years ago
|
||
(In reply to Dustin J. Mitchell [:dustin] from comment #2)
> I think we should just limit this to printable ascii (same a scopes), but
> not limit its length. We don't currently include these identifiers in pulse
> routes, and I don't see why we would.
>
> Jonas, concur?
keeping in eye on it.
Reporter | ||
Comment 4•7 years ago
|
||
We picked GENERIC_ID_PATTERN back in the day, because we can fit those in to pulse routing keys.
I suggested it because I wanted us to publish pulse message whenever hooks are created, modified, deleted or triggered.
If you guys want to use printable ascii (max 255 chars), then I would suggest:
A) merge hookGroupId and hookId into one identifier
B) make pulse exchanges include a sha256(hookId)[0:32] in the routing key
note: totally unrelated, pulse exchanges were never implemented.
Flags: needinfo?(jopsen)
Assignee | ||
Updated•7 years ago
|
Assignee: alesilva241 → dustin
Mentor: dustin
Assignee | ||
Comment 5•7 years ago
|
||
> note: totally unrelated, pulse exchanges were never implemented.
Alex is working on adding this now, and there's really no good reason to include the hookId / hookGroupId in the routing key. So I think we can leave these as they are (well, with some minor restrictions to hookId, which is currently just "string").
Assignee | ||
Comment 6•7 years ago
|
||
Comment 7•7 years ago
|
||
Commit pushed to master at https://github.com/taskcluster/taskcluster-hooks
https://github.com/taskcluster/taskcluster-hooks/commit/a9e03a25e9ef890db3e54b7631728e1508ac013b
Bug 1227606 - allow longer hookGroupId, restrict charset of hookId
This unifies both on 64 characters, alphanumeric plus `-` and `_`. The
idea is that this is more open than 22 characters, but not so open that
we will later want to dial it back.
Assignee | ||
Updated•7 years ago
|
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Component: Hooks → Services
You need to log in
before you can comment on or make changes to this bug.
Description
•