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

RESOLVED FIXED

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jlal, Assigned: jlal)

Tracking

Details

Attachments

(11 attachments, 1 obsolete attachment)

39 bytes, text/x-review-board-request
garndt
: review+
Details
39 bytes, text/x-review-board-request
garndt
: review+
Details
39 bytes, text/x-review-board-request
garndt
: review+
Details
39 bytes, text/x-review-board-request
garndt
: review+
Details
39 bytes, text/x-review-board-request
garndt
: review+
Details
39 bytes, text/x-review-board-request
garndt
: review+
Details
39 bytes, text/x-review-board-request
garndt
: review+
Details
39 bytes, text/x-review-board-request
garndt
: review+
Details
39 bytes, text/x-review-board-request
garndt
: review+
Details
39 bytes, text/x-review-board-request
garndt
: review+
Details
39 bytes, text/x-review-board-request
garndt
: review+
Details
Assignee

Description

4 years ago
No description provided.
Assignee

Comment 1

4 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

4 years ago
/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

4 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 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 on attachment 8561503 [details]
MozReview Request: bz://1130683/lightsofapollo

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

Ship It!
Attachment #8561503 - Flags: review+
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

4 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.
https://hg.mozilla.org/mozilla-central/rev/0e2316747aaf
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Assignee

Comment 12

4 years ago
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+
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.