gecko: Owner validation (email validations) should never cause task graph / push failures

RESOLVED FIXED

Status

Taskcluster
General
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: lightsofapollo, Assigned: lightsofapollo)

Tracking

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(11 attachments, 1 obsolete attachment)

39 bytes, text/x-review-board-request
garndt
: review+
Details | Review
39 bytes, text/x-review-board-request
garndt
: review+
Details | Review
39 bytes, text/x-review-board-request
garndt
: review+
Details | Review
39 bytes, text/x-review-board-request
garndt
: review+
Details | Review
39 bytes, text/x-review-board-request
garndt
: review+
Details | Review
39 bytes, text/x-review-board-request
garndt
: review+
Details | Review
39 bytes, text/x-review-board-request
garndt
: review+
Details | Review
39 bytes, text/x-review-board-request
garndt
: review+
Details | Review
39 bytes, text/x-review-board-request
garndt
: review+
Details | Review
39 bytes, text/x-review-board-request
garndt
: review+
Details | Review
39 bytes, text/x-review-board-request
garndt
: review+
Details | Review
Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
This is an ongoing discussion ... Some of the bots seem to fail the email validator and other commits seem to have irregular emails (in github integration this is the wild west).

I think this comes down to the fact the _only_ the server side integration knows if it should be posting data for X user and that information does not always tie cleanly to a single "owner". For now I am going to set "owner" to a mailing list for tc (or my own). In addition adding the original "owner" flag to:

tags.createdForUser = <user>

<user> will mean github user likely in one case and whatever the value for email we get in the pushlog case.
For the record:
Github suggests that people use: <username>@users.noreply.github.com, if they want to hide their email.
See: https://help.github.com/articles/keeping-your-email-address-private/

We could do the same if github users don't have an email associated.
--
Granted I'm open to changing the requirement for "owner". So that the docs still says that it has to be an email, but that we don't validate this. So obscure owner strings will be accepted.
It's not like we have a strict use-case for it, or some method of validating that the person posted the task really has the given email.

But if we could rely on pseudo emails as above, it is in my opinion nicer to have a field that is guaranteed to be an email. Even if the email may be fake or invalid.
(Assignee)

Comment 3

3 years ago
Created attachment 8561503 [details]
MozReview Request: bz://1130683/lightsofapollo

/r/3593 - Bug 1130683 - Use stable owner name which will never fail

Pull down this commit:

hg pull review -r cbfd20edbf008ff6700885107f0aff4f2fd96727
Attachment #8561503 - Flags: review?(garndt)
(Assignee)

Comment 4

3 years ago
Comment on attachment 8561503 [details]
MozReview Request: bz://1130683/lightsofapollo

/r/3593 - Bug 1130683 - Use stable owner name which will never fail
/r/3595 - Bug 1131152 - Update tester image to use tc-vcs and download mozharness r=garndt
/r/3603 - Bug - 1131154 Use fake buildprops file to bypass use of buildbotconfigs in mozharness r=garndt
/r/3605 - Bug 1130696 - Update pull gaia script to use correct version based on gaia.json in gecko r=garndt
/r/3607 - Bug 1131156 - Add tc-vcs cache to tests r=garndt
/r/3609 - Bug 1124340 - Add buildbot step lines to tester image r=garndt
/r/3611 - Bug 1131160 - Remove caching of gaia due to issues updating clones on tester r=garndt
/r/3613 - Bug - 1131164 Add mozharness generated logs as artifacts r=garndt
/r/3615 - Bug - 1131165 Increase xpcshell timeout to closer to 2 hours r=garndt
/r/3617 - Bug 1131166 - Update mochitest symbol to use tc prefix r=garndt
/r/3619 - Bug 1131169 - Attempt to enable some emulator-kk tests r=garndt

Pull down these commits:

hg pull review -r bd41dd3464778cefa40c547fba8c4fd871da41d6
@jlal, if we're just going to always hardcode and not make the fallback dependent on whether or not we have a valid email for the user. Then we should just drop email validation and let people stuff whatever in owner, but ask (in the docs) that the owner field is an email or something similar.

Comment 6

3 years ago
Comment on attachment 8561503 [details]
MozReview Request: bz://1130683/lightsofapollo

https://reviewboard.mozilla.org/r/3591/#review2821

::: testing/taskcluster/mach_commands.py
(Diff revision 2)
> +    gaia = json.load(open(os.path.join(GECKO, 'b2g/config/gaia.json')))

Probably doesn't matter, but we could just do os.path.join(GECKO, 'b2g', 'config'...) so it uses os.sep as the directory separator

::: testing/taskcluster/mach_commands.py
(Diff revision 2)
> +       gaia['git']['remote'] == '' or \

Not sure it really matter (or if it would happen), but if gaia.json has a key for git, but not for one of the others that this checks for, this will result in a key error being thrown.

::: testing/taskcluster/mach_commands.py
(Diff revision 2)
> -        parameters = {
> +        parameters = dict(gaia_info().items() + {

Is there a situation where gaia.json has 'git' info, but you want it to use the integration branch in hg?
Attachment #8561503 - Flags: review?(garndt)

Comment 7

3 years ago
Comment on attachment 8561503 [details]
MozReview Request: bz://1130683/lightsofapollo

https://reviewboard.mozilla.org/r/3591/#review2831

Ship It!
Attachment #8561503 - Flags: review+

Comment 8

3 years ago
Just a few questions, but have been monitoring try with these changes and as far as I can see so far it looks ok.
(Assignee)

Comment 9

3 years ago
> Probably doesn't matter, but we could just do os.path.join(GECKO, 'b2g', 'config'...) so it uses os.sep as the directory separator

Will fix prior to landing...


>Not sure it really matter (or if it would happen), but if gaia.json has a key for git, but not for one of >the others that this checks for, this will result in a key error being thrown.

I _think_ this is okay as it should fail the decision task in a user visible way.
(Assignee)

Comment 10

3 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/0e2316747aaf
https://hg.mozilla.org/mozilla-central/rev/0e2316747aaf
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Assignee)

Comment 12

3 years ago
Comment on attachment 8561503 [details]
MozReview Request: bz://1130683/lightsofapollo
Attachment #8561503 - Attachment is obsolete: true
Attachment #8619394 - Flags: review+
Attachment #8619395 - Flags: review+
Attachment #8619396 - Flags: review+
Attachment #8619397 - Flags: review+
Attachment #8619398 - Flags: review+
Attachment #8619399 - Flags: review+
Attachment #8619400 - Flags: review+
Attachment #8619401 - Flags: review+
Attachment #8619402 - Flags: review+
Attachment #8619403 - Flags: review+
Attachment #8619404 - Flags: review+
(Assignee)

Comment 13

3 years ago
Created attachment 8619394 [details]
MozReview Request: Bug 1131166 - Update mochitest symbol to use tc prefix r=garndt
(Assignee)

Comment 14

3 years ago
Created attachment 8619395 [details]
MozReview Request: Bug 1131169 - Attempt to enable some emulator-kk tests r=garndt
(Assignee)

Comment 15

3 years ago
Created attachment 8619396 [details]
MozReview Request: Bug 1130683 - Use stable owner name which will never fail
(Assignee)

Comment 16

3 years ago
Created attachment 8619397 [details]
MozReview Request: Bug 1131152 - Update tester image to use tc-vcs and download mozharness r=garndt
(Assignee)

Comment 17

3 years ago
Created attachment 8619398 [details]
MozReview Request: Bug - 1131154 Use fake buildprops file to bypass use of buildbotconfigs in mozharness r=garndt
(Assignee)

Comment 18

3 years ago
Created attachment 8619399 [details]
MozReview Request: Bug 1130696 - Update pull gaia script to use correct version based on gaia.json in gecko r=garndt
(Assignee)

Comment 19

3 years ago
Created attachment 8619400 [details]
MozReview Request: Bug 1131156 - Add tc-vcs cache to tests r=garndt
(Assignee)

Comment 20

3 years ago
Created attachment 8619401 [details]
MozReview Request: Bug 1124340 - Add buildbot step lines to tester image r=garndt
(Assignee)

Comment 21

3 years ago
Created attachment 8619402 [details]
MozReview Request: Bug 1131160 - Remove caching of gaia due to issues updating clones on tester r=garndt
(Assignee)

Comment 22

3 years ago
Created attachment 8619403 [details]
MozReview Request: Bug - 1131164 Add mozharness generated logs as artifacts r=garndt
(Assignee)

Comment 23

3 years ago
Created attachment 8619404 [details]
MozReview Request: Bug - 1131165 Increase xpcshell timeout to closer to 2 hours r=garndt
status-firefox38: fixed → ---
Component: TaskCluster → General
Product: Testing → Taskcluster
Target Milestone: mozilla38 → mozilla41
Version: unspecified → Trunk
Resetting Version and Target Milestone that accidentally got changed...
Target Milestone: mozilla41 → ---
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.