Closed Bug 1194767 Opened 5 years ago Closed 4 years ago

slugids starting with a '-' can cause problems in tools which use them as command line arguments

Categories

(Taskcluster :: Services, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: pmoore, Assigned: pmoore)

References

Details

Attachments

(16 files, 5 obsolete files)

44 bytes, text/x-github-pull-request
pmoore
: review+
dustin
: feedback+
Details | Review
59 bytes, text/x-github-pull-request
jonasfj
: review+
Details | Review
57 bytes, text/x-github-pull-request
pmoore
: review+
Details | Review
60 bytes, text/x-github-pull-request
jhford
: review+
rail
: feedback+
Details | Review
38.73 KB, patch
wcosta
: review+
Details | Diff | Splinter Review
39.21 KB, patch
Details | Diff | Splinter Review
49 bytes, text/x-github-pull-request
bhearsum
: review+
Details | Review
60 bytes, text/x-github-pull-request
jhford
: review+
Details | Review
42 bytes, text/x-github-pull-request
rail
: review+
Details | Review
58 bytes, text/x-github-pull-request
garndt
: review+
Details | Review
57 bytes, text/x-github-pull-request
mrrrgn
: review+
Details | Review
55 bytes, text/x-github-pull-request
jonasfj
: review+
Details | Review
56 bytes, text/x-github-pull-request
jonasfj
: review+
Details | Review
54 bytes, text/x-github-pull-request
garndt
: review+
Details | Review
45 bytes, text/x-github-pull-request
kgrandon
: review+
Details | Review
55 bytes, text/x-github-pull-request
jonasfj
: review+
Details | Review
Slug ids, such as those used to represent taskIds or taskGraphIds, are url-safe. They are also safe to use in AMQP routing keys.

However, when used as command line arguments, they can cause problems if they begin with a '-' which occurs one time in 64, on average. This can introduce an intermittent issue in a command line tool, whereby a failure occurs intermittently.

Tools should work around this, and correctly sanitise their args, however:

a) this can be easily overlooked
b) many command line tools that take slugids as arguments will be outside of our control (i.e. developed by external teams/individuals).

In order to reduce risk, we should prevent slugids that *we* generate starting with a '-' character. For slugids generated by external tools outside our control, (e.g. if a task id is not generating using a slugid utility method we provide) then it should still be allowed. This conforms with the [Robustness principle](https://en.wikipedia.org/wiki/Robustness_principle).

It would not make sense to stray from the base 64 encoding standard (RFC 4648) by using an alternative encoding. It would also not make sense to switch to an alternative encoding (such as hex) since generated ids would be considerably longer, which would negatively impact e.g. routing keys.

We should change this in all language implementations we have (node.js, python, java, go) and also make sure all tests respect use of said method.
The current method of generating a base64 encoding results in a 22 character string, giving a freedom of 22 * 6 = 132 bits. Since the uuid only carries 128 bits, the last character is zero padded with four trailing 0 bits.

The '-' character represents bits 0b111110 (value 62) in base64 and therefore since the last character of the slug is currently 0bxx0000 it means the last character can never be '-' (in reality, the last character must be one of 'A', 'Q', 'g', 'w').

Therefore, I propose a trivial solution to guarantee that the slugId does not being with a '-' character, which is to base64 encode the uuid, and then reverse the sequence of characters in the resulting string. This means all new slugIds will begin with either 'A', 'Q', 'g', or 'w'. This also means there is no loss in randomness (122 bits) by introducing this change.
Attachment #8648742 - Flags: review?(jopsen)
Attachment #8648742 - Flags: feedback?(dustin)
Assignee: nobody → pmoore
+1 on fixing this. Thanks for diving in! I ran into this several times last week.
I've switched the implementation now back to using the old mechanism for generating slugids, but regenerating if a slugid starts with a `-` character. This is so that serialisation and deserialisation still works with existing slugids that were previously created, to avoid breaking anything that might need to decode/encode existing slugids that were generated before this change. It comes at a negligible cost to the randomisation (we lose 1/64th of all possible 122 bit permutations = (2^122)*(63/64) (which is significantly better than losing an entire bit which leave only (2^122)*(1/2) permutations).
I have mixed feelings about this... seems I can't really make up mind on what is sane.

The downside is that we make a intermittent error less frequent, but we don't fix it.
Ideally, people learn how to give command line arguments correctly, and we fix tools to do the right thing(tm).
Not doing this would force people to do so.

On the other hand this would make the bug a lot less likely.
I think we are free to define what the requirements of a slugid are, so I think we could even reject slugs (server side) that did not conform (i.e. they must match `[A-Za-z0-9_][A-Za-z0-9_-]{7}[Q-T][A-Za-z0-9_-][CGKOSWaeimquy26-][A-Za-z0-9_-]{10}[AQgw]`).

Then we rule out all bugs.

In the docs we could define slugId as a url-safe base64 encoded v4 uuid that does not begin with a `-`.

Would that be sufficient?

Being able to consistently use slugids as command line parameters without needing special handling or encoding is a huge plus, IMO, just like it is that we can use them in urls and routing keys.
Agree with pete -- if we could do it over again, I'd suggest hex encoding like version-control sha's, since it automatically avoids any potential punctuation issues.  That said, I don't see `_` causing an issue, so I think that just disallowing an initial `-` is sufficient.

There are a bunch more places where slugids are taken as command-line arguments, across a lot more languages.  Getting tests written to ensure that all of those are doing it "right" and consistently would be difficult.  On the other hand, there are only a few places where slugids are generated, and they're all pretty easy to test.

I would add a reference to Postel's law to the documentation and the implementation, too -- I don't think we should update the schemas to reject `-`-prefixed slugIds, as that will make 1.6% of existing tasks inaccessible.  Instead, the docs should just *recommend* generating slugIds that do not begin with `-` and follow this recommendation in the available implementations.
Comment on attachment 8648742 [details] [review]
TaskCluster SlugID Github Pull Request

See comment on PR, there can be no looping here.
But other than that, I'm coming around to the idea.

Let's define slugids as url-safe base64 encoded v4 uuids stripped if == padding.
With the recommendation that when generating them slugids starting with - are avoided.

Note: We'll need to patch our python client to generate slugids like this too.

---
Once you have the comment addressed refile this and bump version number to say 1.1.0 perhaps...
Attachment #8648742 - Flags: review?(jopsen) → review-
(In reply to Jonas Finnemann Jensen (:jonasfj) from comment #8)
> Comment on attachment 8648742 [details] [review]
> Github Pull Request
> 
> See comment on PR, there can be no looping here.

See my response in the PR:
https://github.com/taskcluster/slugid/pull/1/files#r37629129

Unfortunately this reduced performance, and added complexity. Therefore I think we are better to leave it as is. For example, the chance of having three consecutive uuids that have to be rejected is 1 in 262144 (64^3). This is so remote that overall the performance is better with the loop implementation than the alternative implementation which imports the cryptography library and does the byte manipulation directly in the slugid library. It is arguably safer and cleaner too.


(In reply to Jonas Finnemann Jensen (:jonasfj) from comment #8)
> Comment on attachment 8648742 [details] [review]
> Github Pull Request
> 
> See comment on PR, there can be no looping here.

See my response in the PR:
https://github.com/taskcluster/slugid/pull/1/files#r37629129

Unfortunately this reduced performance, and added complexity. Therefore I think we are better to leave it as is. For example, the chance of having three consecutive uuids that have to be rejected is 1 in 262144 (64^3). This is so remote that overall the performance is better with the loop implementation than the alternative implementation which imports the cryptography library and does the byte manipulation directly in the slugid library. It is arguably safer and cleaner too.

> Once you have the comment addressed refile this and bump version number to
> say 1.1.0 perhaps...

Good spot! Done. =)
Flags: needinfo?(jopsen)
Attachment #8648742 - Flags: feedback?(dustin) → feedback+
See my reply on PR... Basically we should just mask + to something else... hardcoded...
Flags: needinfo?(jopsen)
Using a fixed character ('A') would introduce a skew in the distribution of slugids (twice as many would start with an 'A' as any other character) and would also unnecessarily reduce entropy. If there is a strong reason not to accept my version I will understand, but at the moment I don't understand what the objection to the loop implementation (as I believe we established it is not performance or code readability). Thanks.
Blocks: 1197788
I've updated the PR with an alternative non-looping version which (un)sets the first (most significant) bit of the uuid. This reduces entropy from 122 bits to 121, but avoids the loop. My preference would be the loop.... but you can't win them all. :p

Hoping this will be sufficient to get the issue resolved.
(In reply to Greg Arndt [:garndt] from comment #9)
> We should change this as well:
> https://dxr.mozilla.org/mozilla-central/source/testing/taskcluster/
> taskcluster_graph/slugid.py#4

Good spot! OK, as soon as we're all happy with the node version and it is accepted, I'm happy to port to python.

Jonas suggested we also add a utility method to the taskcluster clients that calls slugid.nice() - that seems to make sense to me too, so I'd like to do this too. However...

One possibility is that the slug creation is hidden by the client - i.e. if you call a method that requires a slug, it just generates one under the hood. But .... I'm guessing since the API methods expect a slug (rather than creating one themselves), we intended that the api consumer might wish to create his/her own slugs - and therefore it would not make sense to hide it in the client, since we intentionally expose it in the API.

For example, http://docs.taskcluster.net/queue/api-docs/#createTask requires that you specify a taskId in the URL you call, rather than assigning a slug and then telling you what the randomly generated slug was (taskId) is in the response. I can imagine this might be intentional so you can refer to entities that don't yet exist without needing to care too much on the order you create them in. However, if this isn't the case, we do have the possibility to hide the slugid generation from the consumer of taskcluster clients, by embedding the slug generation in the taskcluster client api methods that require slugs. Maybe this is a case of trying to simplify things too much, which results in a loss of power to the library user. Another option of course is making the slugid optional, and if you don't specify one, one gets generated for you.

Your thoughts please, good people of the world...
Flags: needinfo?(jopsen)
> One possibility is that the slug creation is hidden by the client
For the exact reasons you mention, this is a bad idea.
In particular idempotency is only fun if you can control the input.
Simply put you generate the slugid because that way you can have fault tolerance through eventual consistency.

Note, you should always create in correct order, to prevent an attacker from stealing a
slugid you intended to use. This could result in a security issue, in some cases.

------
On topic, I suggest we make it so that:
  var taskcluster = require('taskcluster-client');
  taskcluster.slugid() or taskcluster.newTaskId(), or taskcluster.generateId() or
  taskcluster.generateTaskId()

I suspect taskcluster.slugid() is the best method name. Then that can just run slugid.nice() under the
hood. That way devs only have to require the taskcluster-client library, and not the slugid library.
(so it also simplifies dependency management, pretty sure tc-client already needs slugid anyways).

I suggest we do exactly the same thing in both python, go and java clients. We don't need a separate
slugid library we can just make it part of the client library (assuming that's easier).
Flags: needinfo?(jopsen)
Comment on attachment 8648742 [details] [review]
TaskCluster SlugID Github Pull Request

Updating from r- to r+ to reflect Pull Request history.
Attachment #8648742 - Flags: review- → review+
Just a doc update to recommend use of slugid.nice() (previously slugid.v4()).
Attachment #8654079 - Flags: review?(jopsen)
Attachment #8648742 - Attachment description: Github Pull Request → TaskCluster SlugID Github Pull Request
I've made the patch for gecko, but I haven't quite worked out how to run the python unit tests for taskcluster_graph so I haven't added tests yet. As it is end-of-day, I'll attach the patch without tests, and will add a second patch with tests tomorrow (once I work out how to run them - see https://irccloud.mozilla.com/pastebin/kbLfPIyG).
Attachment #8654187 - Flags: review?(garndt)
Attachment #8654187 - Flags: feedback?(jopsen)
Also need to land a patch for gaia - I'll do this tomorrow.
P.S. agree with comment 15, I'll proceed as suggested there.
Comment on attachment 8654079 [details] [review]
TaskCluster Scheduler Github Pull Request

r+ with comment about leaving prototype warning in there.
Attachment #8654079 - Flags: review?(jopsen) → review+
Comment on attachment 8654187 [details] [diff] [review]
Gecko patch (for taskcluster_graph) - without tests

Looks decent to me...

@pmoore, please add the same logic for python client.
I think it having a slugid() method makes sense too (if it doesn't already).
For consistency, let's call it slugid().
Attachment #8654187 - Flags: feedback?(jopsen) → feedback+
taskcluster.slugid() for taskcluster-client
Attachment #8654247 - Flags: review?(pmoore)
Comment on attachment 8654247 [details] [review]
github PR for tc-client

r+ with the review comments addressed
Attachment #8654247 - Flags: review?(pmoore) → review+
I started work on a python port of slugid, since lots of places might want to use the library, including:

a) gecko
b) gaia
c) buildbot bridge
d) python taskcluster client
e) any project using the taskcluster-github integration that Morgan wrote

This would be preferable to having the logic in multiple python projects.

It's end-of-day for me now, so here is the work in progress for anyone interested:
https://github.com/taskcluster/slugid.py/compare/master...petemoore:initial-dev

Will raise the PR when finished with tests added etc.
Attachment #8654187 - Flags: review?(garndt) → review+
Depends on: 1201062
Now that I've created a standalone slugid python library (in bug 1201062) I've redone this patch to use it.

See https://pypi.python.org/pypi/slugid.
Attachment #8654187 - Attachment is obsolete: true
Attachment #8656536 - Flags: review?(wcosta)
Attachment #8656536 - Flags: feedback?(garndt)
Comment on attachment 8656536 [details] [diff] [review]
Gecko patch (for taskcluster_graph) to use new slugid.py library

Adding gps, based on:
https://hg.mozilla.org/mozilla-central/rev/21f18a7f5f9d
Attachment #8656536 - Flags: feedback?(gps)
Comment on attachment 8656536 [details] [diff] [review]
Gecko patch (for taskcluster_graph) to use new slugid.py library

Moving the logic into it's own package so things like taskcluster-client.py, mozci and anything else python related generating slugids for tasks makes sense.

however, we need to figure out how to publish this and get it include when mach is invoked.  (discussions related to it on IRC).
Attachment #8656536 - Flags: feedback?(garndt) → feedback+
Adding Rail to f? as one of the primary consumers of taskcluster-client.py...
Attachment #8656567 - Flags: review?(jhford)
Attachment #8656567 - Flags: feedback?(rail)
Attachment #8656567 - Flags: feedback?(rail) → feedback+
Comment on attachment 8656536 [details] [diff] [review]
Gecko patch (for taskcluster_graph) to use new slugid.py library

Review of attachment 8656536 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the deployment issue fixed.
Attachment #8656536 - Flags: review?(wcosta) → review+
Comment on attachment 8656567 [details] [review]
Github Pull Request for taskcluster-client.py

Can we rename TestSlugIdNotNice to TestSlugIdV4?

Otherwise, looks good to me, r+
Attachment #8656567 - Flags: review?(jhford) → review+
(In reply to John Ford [:jhford] from comment #32)
> Comment on attachment 8656567 [details] [review]
> Github Pull Request for taskcluster-client.py
> 
> Can we rename TestSlugIdNotNice to TestSlugIdV4?
> 
> Otherwise, looks good to me, r+

Yeah, the name isn't good, I agree. I called it "NotNice" to reflect that first bit is *set*, whereas with v4 the bit may or may not be set. A v4 slug can also be a nice slug (50% chance). Can you think of a another name we could give it that implies the first bit is set?
Flags: needinfo?(jhford)
Component: General → Platform Libraries
Updated mozilla-central patch to include slugid 1.0.6 library in /python.

Tested locally with:

  $ ./mach -v taskcluster-graph --pushlog-id='17316' --message=' ' --project='b2g-inbound' --owner='ffxbld' --revisionhash='f572e2439068601d4f7573955760d9ffa8a7442b' --extend-graph

which I stole from:

  * https://queue.taskcluster.net/v1/task/7TnNDeMzRHSKium5NF1ycQ
Attachment #8656536 - Attachment is obsolete: true
Attachment #8656536 - Flags: feedback?(gps)
Attachment #8657085 - Flags: review?(wcosta)
Attachment #8657085 - Flags: review?(wcosta) → review+
^^^ In this try run, I set all the taskcluster build tasks to be:

  command: ["/bin/bash", "-c", "true"]

in order that I could test creating a graph, without needing to run all the jobs and waste compute resources.
Yay, all tasksIds (and the group id) begin with [A-Za-f] \o/

https://tools.taskcluster.net/task-graph-inspector/#OgMQE_rXSbOzn0hcaFqjZg/
Forgot to include the commit header - this patch is identical to last, just with hg commit header.

I'll ask a sheriff to land it. :)
"checkin-needed" only for the last patch (attachment 8657110 [details] [diff] [review]).
Keywords: checkin-needed
(In reply to John Ford [:jhford] from comment #32)
> Comment on attachment 8656567 [details] [review]
> Github Pull Request for taskcluster-client.py
> 
> Can we rename TestSlugIdNotNice to TestSlugIdV4?
> 
> Otherwise, looks good to me, r+

Discussed on IRC, we agreed "TestSlugIdNiceIsAlwaysNice". Will make the change, and merge the PR.
Flags: needinfo?(jhford)
Hey pete, this didn't apply to mozilla-inbound 

renamed 1194767 -> bug1194767_mozilla-central_v2_with_commit_header.patch
applying bug1194767_mozilla-central_v2_with_commit_header.patch
patching file testing/taskcluster/mach_commands.py
Hunk #1 FAILED at 251
1 out of 1 hunks FAILED -- saving rejects to file testing/taskcluster/mach_commands.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh bug1194767_mozilla-central_v2_with_commit_header.patch

maybe need a rebase ?
Flags: needinfo?(pmoore)
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #41)
> Hey pete, this didn't apply to mozilla-inbound 
> 
> renamed 1194767 -> bug1194767_mozilla-central_v2_with_commit_header.patch
> applying bug1194767_mozilla-central_v2_with_commit_header.patch
> patching file testing/taskcluster/mach_commands.py
> Hunk #1 FAILED at 251
> 1 out of 1 hunks FAILED -- saving rejects to file
> testing/taskcluster/mach_commands.py.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and refresh
> bug1194767_mozilla-central_v2_with_commit_header.patch
> 
> maybe need a rebase ?

Rebased against mozilla-inbound. Sorry about that! This should be good to land against inbound now.
Attachment #8657110 - Attachment is obsolete: true
Flags: needinfo?(pmoore)
"checkin-needed" only for the last patch (attachment 8657173 [details] [diff] [review]).
Keywords: checkin-needed
Need this so we can republish to npm, and use that version e.g. in gaia.

Gaia is also using slugid library directly, but I'll be bumping both, to be safe and more future-proofed...

Please republish on npm if you merge it (I think I don't have permission to do that step?).

Thanks!
Attachment #8657202 - Flags: review?(jopsen)
Comment on attachment 8657202 [details] [review]
taskcluster-client PR with bumped version

Apologies, I see it was already bumped since we updated. No need for a new release.
Attachment #8657202 - Attachment is obsolete: true
Attachment #8657202 - Flags: review?(jopsen)
See Also: → 1201951
Depends on: 1201951
See Also: 1201951
Buildbot Bridge doesn't generate slugids during regular operation (listening only to notifications from preexisting tasks) but its tests do require slugids. This PR aligns the test slugs to how we now generate production slugs.
Attachment #8657597 - Flags: review?(bhearsum)
https://hg.mozilla.org/mozilla-central/rev/e3bd50974b08
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
These are some enhancements to the release.sh script which I'd like to add before I make release 0.0.26.

Any questions, let me know. :)
Attachment #8657821 - Flags: review?(jhford)
And while I think of it, also ...

* https://github.com/taskcluster/taskcluster-docs (everywhere where slugs are defined, and have regexs)
* https://github.com/taskcluster/taskcluster-tools (e.g. task creator)
* publish new schemas/references with updated comments for slug fields
> * https://github.com/mozilla/build-funsize

I *think* this has been superseded by https://github.com/mozilla/funsize

Rail, can you confirm? Thanks!
Flags: needinfo?(rail)
Also it looks like it breaks docker worker, so no doubt we will need a fix for docker worker so that it can manage taskids / taskgroupids starting with a '-' even if we don't create them by default.

e.g. this failure I believe is because docker worker doesn't handle task group ids starting with a '-':

https://tools.taskcluster.net/task-inspector/#2AAxnGTzSeGLTX_Hwp_PVg

In other words, we shouldn't only fix clients to not use '-', we should also make sure we can handle them if we receive them (or block them via the API with appropriate regex).
Attachment #8657866 - Flags: review?(winter2718)
(In reply to Pete Moore [:pmoore][:pete] from comment #52)
> > * https://github.com/mozilla/build-funsize
> 
> I *think* this has been superseded by https://github.com/mozilla/funsize
> 
> Rail, can you confirm? Thanks!

Rail confirmed via IRC, see bug 1202457 about deprecating the old repo.
Flags: needinfo?(rail)
See Also: → 1202457
(In reply to Pete Moore [:pmoore][:pete] from comment #51)
> And while I think of it, also ...
> 
> * https://github.com/taskcluster/taskcluster-docs (everywhere where slugs
> are defined, and have regexs)
> * https://github.com/taskcluster/taskcluster-tools (e.g. task creator)
> * publish new schemas/references with updated comments for slug fields

Jonas, Dustin,

I'm tempted to suggest we update our regexes in our schemas/references to the 'nice' regex from taskcluster.github.io/slugid.py for creation of *new* objects:
[A-Za-z0-9_-]{8}[Q-T][A-Za-z0-9_-][CGKOSWaeimquy26-][A-Za-z0-9_-]{10}[AQgw]

... and to update the regexes to the 'v4' regex for api endpoints that query existing data:
[A-Za-f][A-Za-z0-9_-]{7}[Q-T][A-Za-z0-9_-][CGKOSWaeimquy26-][A-Za-z0-9_-]{10}[AQgw]

This should add some safety checks that provided slugids really are uuid v4 compliant, and that new entities are also 'nice'.

Currently we use:
[A-Za-z0-9_-]{22}

which is not a guarantee that the slug is v4 compliant, and thus could potentially introduce a security risk for a taskcluster component at a point in the future, which relies on it being so.
Flags: needinfo?(jopsen)
Flags: needinfo?(dustin)
^^^ above, I put the regexes in the wrong order - of course the 'nice' slugs start with [A-Za-f] and the v4 ones start with [A-Za-z0-9_-]{8} ^^^

(was a long day) ;)
@pmoore,
I support upgrading schemas and regexp for URLs to v4-regexp:
  [A-Za-z0-9_-]{8}[Q-T][A-Za-z0-9_-][CGKOSWaeimquy26-][A-Za-z0-9_-]{10}[AQgw]
Instead of using:
  [A-Za-z0-9_-]{22}
Please go a head with this for queue, scheduler, etc..

But the definition of slugid remains the same. We just recommend that people use the "nice" form.
We don't require it -- some day if we ever make v2/, we can discuss a completely new encoding.
So I don't think we should use the "nice"-regexp, just the v4-regexp which is a good improvement.
Flags: needinfo?(jopsen)
See Also: → 1202573
(In reply to Jonas Finnemann Jensen (:jonasfj) from comment #60)
> @pmoore,
> I support upgrading schemas and regexp for URLs to v4-regexp:
>   [A-Za-z0-9_-]{8}[Q-T][A-Za-z0-9_-][CGKOSWaeimquy26-][A-Za-z0-9_-]{10}[AQgw]
> Instead of using:
>   [A-Za-z0-9_-]{22}
> Please go a head with this for queue, scheduler, etc..

Created bug 1202573.
no info needed
Flags: needinfo?(dustin)
Comment on attachment 8657858 [details] [review]
Github Pull Request for mozilla-taskcluster

Looks, good.  Merged and deployed.
Attachment #8657858 - Flags: review?(garndt) → review+
Attachment #8657597 - Flags: review?(bhearsum) → review+
(In reply to Dustin J. Mitchell [:dustin] from comment #62)
> no info needed

Sorry - this was my lazy way of asking if you think the suggestion makes sense. :)
Attachment #8657866 - Flags: review?(winter2718) → review+
Comment on attachment 8657856 [details] [review]
Github Pull Request for funsize

merged
Attachment #8657856 - Flags: review?(rail) → review+
Attachment #8657821 - Flags: review?(jhford) → review+
Almost done now...
Attachment #8659262 - Flags: review?(jopsen)
Hopefully this is the last one, and then we can close the bug! \o/
Attachment #8659263 - Flags: review?(jopsen)
(In reply to Dustin J. Mitchell [:dustin] from comment #7)

> On the other hand, there are only a few places where slugids are generated,
> and they're all pretty easy to test.

Famous last words!! :D
Comment on attachment 8659262 [details] [review]
Github Pull Request for taskcluster/taskcluster-docs

r+ with comments on quickly moving to taskcluster.slugid() instead of slugid.nice()
Attachment #8659262 - Flags: review?(jopsen) → review+
Comment on attachment 8659263 [details] [review]
Github Pull Request for taskcluster/taskcluster-tools

Landed, thanks..
Attachment #8659263 - Flags: review?(jopsen) → review+
\o/
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
It seems we missed:

* https://github.com/taskcluster/gaia-taskcluster/blob/18d38dc6bdd28081b2c26306c571422067e8b2d5/routes/github_pr.js#L167
* https://github.com/mozilla/autolander/blob/dea6e36d1dcbbc252494512a61277d5deac2417b/lib/taskgraph.js#L270

Thanks to :garndt for the links above!

Will make patches for these two tools too.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8678113 - Flags: review?(garndt)
Attached file Github Pull Request for autolander (obsolete) —
Attachment #8678122 - Flags: review?(kevingrandon)
Comment on attachment 8678122 [details] [review]
Github Pull Request for autolander

Autolander also created an attachment, so obseleting mine (they had the same content).
Attachment #8678122 - Attachment is obsolete: true
Attachment #8678122 - Flags: review?(kevingrandon)
Attachment #8678119 - Flags: review?(kevingrandon)
Comment on attachment 8678113 [details] [review]
Github Pull Request for gaia-taskcluster

Looks reasonable to me.
Attachment #8678113 - Flags: review?(garndt) → review+
Have redeployed gaia-taskcluster with the changes. N.B. there were a couple of commits in heroku that weren't on github. I checked they had no sensitive changes, and merged them in. The added commits were:

https://github.com/taskcluster/gaia-taskcluster/compare/18d38dc6bdd28081b2c26306c571422067e8b2d5...3329875f1481b9216dfeb35898945468c63fe1a4

These had not been published to github before.
Comment on attachment 8678119 [details] [review]
[autolander] petemoore:bug1194767 > mozilla:master

Seems fine to me, thanks!
Attachment #8678119 - Flags: review?(kevingrandon) → review+
Awesome, thanks!
Status: REOPENED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
Attachment #8983497 - Flags: review?(jopsen)
Attachment #8983497 - Flags: review?(jopsen) → review+
Component: Platform Libraries → Services
You need to log in before you can comment on or make changes to this bug.