mozilla :: #conduit

11 Jul 2017
01:17marsimadueme|afk: argh, I absolutely remember r+'ing the requirements refactoring and another PR last week. I even remember talking about it in IRC. But I don't see my 'LGTM' in GH :(
01:18marsimadueme|afk: the requirements refactoring is approved now, merge away
01:25imadueme|afkmars: that was for the ui. I did the same on the api. No worries, thanks!
13:15marsmorning zalun
13:15marszalun: I updated the mozlog PR. I think it's done, please have a look when you have time?
13:16marszalun: just saw the r+: I had to push a rebase, but it shouldn't affect anything
14:38marshi guys, I need to do my standup today via IRC
14:39marsThe mozlogging PR is in review, I think it's done and can land. It's also blocking 2 other branches, so I'd like to merge it today.
14:39marsI'm going to work on Sentry migration today.
14:40marsno blockers for this sprint
14:47zalunI'd like to do it via irc as well
14:47dklstandups: continuing to try different paths with regard to security policy. looking at davidwalsh diff for the push connector. pushed PR yesterday to ckolos for docs building on dev instance.
14:47zalunDid some reviews, started to work on saving the diff_phid in the lando-api database
14:48davidwalshI'm looking at imadueme's PRs now
14:54imaduememy status is that I finished the auth0-to-ui integration (see the gif in the backlog) and now am working on the card to confirm if the commit email is stored somewhere in the phabricator database.
14:56imaduemesorry mars: can you rebase for the new requirements changes?
14:58imaduemealso zalun: :P
15:33marsimadueme: sure, np
15:33imaduemethanks, in hindsight i should have merged your first then rebased mine :)
16:15mcoteckolos: smaug reports that he can't load from where he is (Scandinavia I believe)
16:15ckolosinteresting; I am aware of nothing that should stand in the way of the site being loaded from anywhere
16:17ckolosseems unhappy from the netherlands too
16:19ckolosah, loads now
16:24ckolosmcote: he's working again
16:32mcoteckolos: just a random network blip?
16:32ckolosguessing so; no other discernable reason for it
16:32ckolosit reloaded just fine for him on the first try
16:32ckolos(while I was chatting with him)
16:33ckolosI blame the internet
17:10davidwalshdkl: Any thoughts yet?
17:26imaduemeGood news everyone! Phabricator actually does expose the commit author's email address (from their hgrc settings, of course only if you use arc diff). It turns out I was just blind when I looked for it (because I actually looked for the Phabricator user's email address, not the commit author's). Oops :D
17:26smacleodimadueme: what happens when there are multiple commits?\
17:26imaduemesmacleod: standby
17:28davidwalshsmacleod: We're disabling multiple commits
17:28smacleoddavidwalsh: uhhhhh that's a no go? what do you mean?
17:28smacleodmultiple commits is 1--% required
17:28davidwalshsmacleod: Banter isn't as satifying remotely, is it?
17:29smacleodbah.... at least give me an emoticon
17:31imaduemeFor this hg log:
17:32imaduemePhabricator returns all 5 email addresses for the 5 commits as:
17:33imadueme^ smacleod. That being said, can't transplant only use 1 author since it makes just 1 patch file?
17:33smacleodimadueme: correct, that's the issue here, in multiple commit case we need to pick an author to use
17:34imaduemesmacleod: I'm still unsure why we cannot just send 5 patch files to transplant and have it do it's thing
17:34imaduemeThere's only 1 in phabricator anyway :)
17:35mcoteyeah we're using the squash model here
17:35mcoteyeah, probably the author from the last commit
17:36smacleodI think that's a pretty poor solution tbh ^
17:36smacleodmajority makes more sense to me
17:36mcoteI dunno, seems like the final word should be what we go with
17:36mcotelike if someone takes over a patch at the end
17:36smacleod"the final word"?
17:36mcoteif I put up two commits and then for whatever reason you take over and put up a last commit, I think that's the final word
17:36mcoteyou are now taking ownership of the work
17:37smacleodI don't think that's the only case here
17:37mcotemost relevant if a contributor disappears or something
17:37imadueme(on a side note smacleod, dkl: The commit author is only available if you use arc diff, which is another reason to disable the web based patch submission imo)
17:37mcotefeels like the common case here
17:37mcoteI think we should do that and then if we want have a selection in the UI
17:37mcotemajority doesn't seem right to me
17:38smacleodreally the "author" should be whoever wrote most of what is landing here
17:38smacleodbut that can't be gathered as we don't know which parts are from each commit
17:38smacleodso my thinking is majority commits wins
17:38imaduemeI also would prefer the tip. It is predictable. Unless it is firefox devs commonly submit patches with mixed authors
17:39smacleodHow about this, we go with commit author if it's unanimous, if it's not unanimous, but at least one commit author matches phabricator email, go with that, otherwise go with phabricator email?
17:39imaduemeit is also (very) difficult to measure how "large" each commit is. So even if there are 10 commits by Author1, the last commit might be 20x the size.
17:40smacleodimadueme: that was my point
17:40smacleod"but that can't be gathered as we don't know..."
17:40imaduemeJust emptying my buffer, didn't see your message yet :)
17:40* smacleod isn't opposed with going with tip for now though - it's the simplest
17:40smacleodif we end up having problems, push to review will fix this anyway
17:40smacleodsort of
17:41* smacleod still kinda thinks using the phabricator user makes the most sense
17:43imaduemesmacleod: I like commit email == phab email, and default to tip otherwise.
17:43imaduemethen we should just tell people to put their ldap email as a secondary address in phab if it isn't there already. but it won't be a critical part of the workflow like planned before.
17:44smacleodimadueme: other option is instead of comparing commit email to phab email, compare commit author *name* to phabricator *name*
17:44smacleodand if they match, use the email in the commit
17:45imadueme=== email, === name, tip?
17:45smacleodgets around the whole "" problem
17:45mcotethis is getting complicated
17:45smacleodimadueme: sounds fine to me... should be easy to check too right - phabricator is exposing the author's "Full Name" in phabricator to you right?
17:45mcotewhat is the main reason against tip?
17:46mcotelet's just do tip right now
17:46mcotewe can make it more complicated later if we need
17:46mcotethe advantage of this method is that it's easy to understand and easy to fix (don't like the author? push up a new commit)
17:49imaduemePhabricator exposes the name as defined in .hgrc in the commit list, and the "Full name" of the phabricator user who created the revision. If we wanted to compare those.
17:49imaduememcote: I'm in favor of tip, but, it would be nice to match the user who actually submitted the revision to the final commit that gets pushed to m-central. That's how I'm seeing it (and in hopefully 90% mixed authors isn't a thing?)
17:50imaduemeAnd so far it is much less complicated that the original plan to create some email extension. I'm glad we made the card to investigate this again :)
17:50mcoteisn't that the same time? final revision and tip?
17:50mcotelike if there are 5 commits in one revision, we use the author from revision 5
17:52imaduememcote: I honestly have no idea what version control shenanigans firefox devs do :). I'm imagining someone having 4 commits ready, cherry picking some commit from a friend in Australia, running arc diff, and then the tip will be "blog" instead of "imadueme" haha.
17:52imaduememcote: this isn't about multiple revisions, it is about 1
17:53mcoteyes I know
17:53imaduemeoh so you mean commit 5 above?
17:53mcoteah sorry I meant "the author from commit 5"
17:53imaduemegot it
17:53imaduememcote: do you think stuff like that happens?
17:53mcoteI bet rarely
17:53* smacleod is less sure about rarely here
17:53mcoteand if it does, the solution is pretty simple: do one more commit with your author string
17:54smacleodit happened with the servo overlay stuff glob worked on in fact
17:54smacleodhe had a large commit set, and cherry picked a couple of my testing commits on top
17:55mcoteand they all got squashed?
17:55smacleodmcote: is there a reason you're opposed to the extra ~3 lines of python complexity to compare the commits against the phabricator user?
17:55mcoteit's harder to understand
17:55smacleodmcote: one of my changes was rolled in, the others were on the top of the stack. Not squashed because MozReview
17:56smacleodmcote: I think it's more intuitive tbh, that the author's changes land with the user string they were using themself, if it exists in one of the commits
17:56imaduememcote: On lando-ui's side the user won't have to even worry about what is going on since it will display the commit author that will be used before landing
17:56mcoteyeah so that's not comparable then. I have a hard time imagining a scenario where I have a commit, and then I pull in someone else's (unrelated?) commit to be squashed together
17:56mcotethe scenario seems very weird
17:56imaduemewait: why don't we just give them the option to choose in lando ui mcote, smacleod
17:56smacleodmcote: it wouldn't be unrelated.. and you're not pulling it in to squash it, your pulling it in to land / review it with your own change
17:56imaduemethat wouldn't be hard
17:56mcoteimadueme: I would prefer that, yes
17:57smacleodthe squashing is a side effect of shittily not preserving commits
17:57mcotesmacleod: but a revision is all squashed
17:57smacleodimadueme: a drop down if there is more than one author of the commits makes sense to me
17:58mcoteyes, make a card for that, do it later
17:58mcotedo tip for now
17:58imaduemeI think choose in lando ui and default to tip is the similest :), prevents the user from having to do weird tip commits. And we can always improve the default selection down the road with the comparison stuff.
17:58smacleodimadueme: if commit data wasn't sent up though, just use the phabricator email?
17:58imaduemeI'll make a card :)
18:21dkldavidwalsh: sorry bout earlier. forgot to change my nick when i went to lunch and then had internet/router issues on return that had to figure out.
18:22dkldavidwalsh: i do have comments but i think it would be easier if we did it over a PR as I cannot comment inside a gist it seems
18:22dklunless you want me to just email you some freeform comments.
18:22davidwalshI can WIP it
18:22davidwalshThe questions I sent you would be helpful though, re: testing
18:22davidwalshBut yeah, I'll WIP it in a moment
18:23dklbut to answer your question yesterday, the functions you need are in extensions/PhabBugz/lib/
18:23dklas for testing, you can just use the demo conduit env as I do and create a private bug in the bugzilla systems. you can use the core-security group for now.
18:24dkland then create a revision in the phab system that references that bug.
18:25dklproblem is you will need to do the full setup as outlined here
18:26dklwhat i need to do is after doing all of the steps in that doc, do a db dump and update the demo env to have them already configured when bringing up a new env
19:11davidwalshdkl: Do you need to do these setup steps every time you docker-compose up?
19:24dkldavidwalsh: no. just if you blow away the DB docker volume
19:25dkldavidwalsh: demo_phabricator-mysql-db
19:25dkldont do 'docker volume rm `docker volume ls -q`
19:50circleci-botSuccess: dklawren's build (#49; push) in mozilla-services/mozphab (master) --
19:57zalunimadueme|lunch: rebased and pushed, I'm not holding to API-Key vs Transplant-API-Key.
20:02dkldavidwalsh: fyi, we will need a bug created for the wip at some point as it is a PR against BMO
20:02imaduemegreat! thanks, no problem about the name. there is a lint error though zalun on circle ci :O
20:05davidwalshdkl: Gah, so many workflooooows
20:06dkli knooooows
20:09smacleodzalun / imadueme: glob may want a particular name
20:10zalunglob: how would you like to name the API-Key header parameter? (is it parameter?)
20:13smacleodzalun: an email may be best, as he's most certainly asleep - also that question doesn't contain enough information, you'll want to be clear about what api key you're referring to and what it is used for
20:14zalunI'll ask him when he will get up.
20:15* smacleod nods
20:15smacleodzalun: it's important to keep him in the loop with these changes as he'll be making the changes to transplant on the other side
20:15zalunor actually before he will go to bed :)
20:15zalunI know - last time he was thinking about OAuth
20:25marsimadueme: is rebased and ready to land. Do you want to look again before I land it, or should I just go ahead?
20:31davidwalshdkl: In step 14 of "Harbormaster Setup", is that the Bugzilla API key or Phab key? I assume Bugzilla?
20:33imaduememars: 1 comment on that PR, feel free to merge and fixup in a new patch or fix now
20:36mcoteokay going to publish this damn post
20:42marsimadueme: fixed
20:42marsCI running, but it should pass
20:46dkldavidwalsh: sorry. yes it is the BMO API key
20:47davidwalshJust got everything set up
20:49dkldavidwalsh: ok cool. do me favor. run 'docker-compose run phabricator dump > demo.sql' and send that to me
20:49dkli would do it on mine but i have a lot of test revisions that i would rather not be included in the bootstrap dump file.
20:51imaduememerged mars :)
20:53marscool, thanks
20:54imaduemezalun: sorry another merge conflict with mars :(
20:54zalunI like them
21:14davidwalshdkl: sent
21:14davidwalshHopefully I did it all right
21:15dkli want to update and the BMO test system as well so everything matches up
21:16dklso hopefully we would not have to do anything to get it all working
12 Jul 2017
No messages
Last message: 12 days and 19 hours ago