Status

Release Engineering
General
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: armenzg, Assigned: lsblakk)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [trychooser], URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
I know there are other improvements to come but we have been asked that imposing the syntax could improve the load on the try server and help with the terrible wait times until the new test pool comes up.
(Reporter)

Updated

7 years ago
Assignee: nobody → lsblakk
(Reporter)

Updated

7 years ago
(Assignee)

Comment 1

7 years ago
So in order to try this, with the option to backout if it's too chaotic, instead of blocking on the interactive chooser - I will write a patch so that a push to try with no syntax generates a different email-on-push than what is currently created.

if you push with try syntax: you will get your builds, normal email (though there is a bug on having more info about what you're going to get in that initial email)

if you push without try syntax: you will get an email that says "try syntax not present" or something like that - No Builds will come of this, basically, and then links to trychooser and try_syntax wiki page to help guide the pusher.
(Assignee)

Comment 2

7 years ago
look at doing it as a hook instead of in the scheduler/email side of things - see bug 559964 where some of try parser is being incorporated.
(Assignee)

Comment 3

7 years ago
Created attachment 532467 [details] [diff] [review]
hg hook to make try syntax mandatory

There's no reason to implement any of the try_parser code here since even just using try: gets you a default set and I think that's enough to get the habit of using try syntax happening.  It will reduce unnecessary try talos runs at minimum, but also hopefully the links provided by the hook will get people looking at the trychooser and starting to think about what they need to request builds on.

The only issue with this hook is that it can only look at the tip changeset, so that means try syntax has to be present in the topmost commit...some people who currently use try and try syntax might find this frustrating to their current usage but I'm not sure how to get around it.
Attachment #532467 - Flags: review?(catlee)
(Assignee)

Comment 4

7 years ago
Created attachment 532468 [details]
hg hook to make try syntax mandatory

this is the correct version of the hook I was testing. It will not work with 'Try: ' (case sensitive) and it does work with multi-line commit messages.
Attachment #532467 - Attachment is obsolete: true
Attachment #532467 - Flags: review?(catlee)
Attachment #532468 - Flags: review?(catlee)

Updated

7 years ago
Attachment #532468 - Attachment is patch: false
Comment on attachment 532468 [details]
hg hook to make try syntax mandatory

If all you're looking for is "try:" in the comments, it would be much easier to do

if "try:" in repo.changetxt('tip').description():
Attachment #532468 - Flags: review?(catlee) → review-
(Assignee)

Comment 6

7 years ago
Created attachment 533725 [details] [diff] [review]
take 2 - make try mandatory with an hg hook

you're right! | if "try: " not in | is much easier and concise.  

so this works, and it gives links to the info you might need if you were surprised by the "try syntax use is now mandatory" response.
Attachment #532468 - Attachment is obsolete: true
Attachment #533725 - Flags: review?(catlee)
Attachment #533725 - Flags: feedback?(ted.mielczarek)

Updated

7 years ago
Attachment #533725 - Flags: review?(catlee) → review+
Comment on attachment 533725 [details] [diff] [review]
take 2 - make try mandatory with an hg hook

Seems reasonable.
Attachment #533725 - Flags: feedback?(ted.mielczarek) → feedback+
(Assignee)

Comment 8

7 years ago
Note to self - add a line saying "We're trying not to waste CPU cycles anymore, please use try syntax to ask for exactly what you want"
per irc with ehsan, catlee, jprmc: 

The objective here is to reduce load on testpool. Our feeling is that most people do not need "-a/-all", and are using so incorrectly. Therefore, please remove the "-a" option in trychooser, and people who do really need all, can specify using the longer syntax.

This can be done here, as part of this rollout, or can be broken out into separate followup bug... whatever makes most sense to lsblakk (doing the actual work!)
Created attachment 537513 [details] [diff] [review]
removing the -a option from try_parser
Attachment #537513 - Flags: review?(catlee)
(Assignee)

Updated

7 years ago
Depends on: 662755
I've updated the trychooser site to remove the -a option, will land that fix in try_parser once catlee reviews it.  Also landed the hook and filed a bug for IT to enable it.
(Assignee)

Updated

7 years ago
Attachment #537513 - Flags: review?(catlee) → review?(rail)
Attachment #537513 - Flags: review?(rail) → review+
Comment on attachment 537513 [details] [diff] [review]
removing the -a option from try_parser

Landed on default, updated https://wiki.mozilla.org/ReleaseEngineering/TryChooser to remove references to -a/--do-everything (and did some other updating).  This is ready to be swept up in the next reconfig.

http://hg.mozilla.org/build/buildbotcustom/rev/68f3606bd6a9
Attachment #537513 - Flags: checked-in+
Created attachment 538033 [details] [diff] [review]
adding a couple of tests to make sure try_mandatory.hook does what it should
Attachment #538033 - Flags: review?(ted.mielczarek)
Comment on attachment 538033 [details] [diff] [review]
adding a couple of tests to make sure try_mandatory.hook does what it should

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

Looks good! In the future, you don't need to get review on test changes, feel free to just push them (as long as the tests pass).
Attachment #538033 - Flags: review?(ted.mielczarek) → review+
With this change what's the syntax that will be required to run all the tests that are run on mozilla-central?

Comment 16

7 years ago
(In reply to comment #15)
> With this change what's the syntax that will be required to run all the tests
> that are run on mozilla-central?

try: -b do -p all -u all -t all
(In reply to comment #16)
> (In reply to comment #15)
> > With this change what's the syntax that will be required to run all the tests
> > that are run on mozilla-central?
> 
> try: -b do -p all -u all -t all

Isn't that just the same as try: -a?

Comment 18

7 years ago
(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > With this change what's the syntax that will be required to run all the tests
> > > that are run on mozilla-central?
> > 
> > try: -b do -p all -u all -t all
> 
> Isn't that just the same as try: -a?

try: -a will be removed, IINM.
So the goal is to save work under the assumption that 'try: -b do -p all -u all -t all' is harder to remember than 'try: -a' causing people to think more?

Comment 20

7 years ago
(In reply to comment #19)
> So the goal is to save work under the assumption that 'try: -b do -p all -u all
> -t all' is harder to remember than 'try: -a' causing people to think more?

Most people do not need to use try: -a.  The goal is to make sure that people do not use it without thinking (or realizing) what it means.  If you need to really get all results, you can just use the longer syntax (which is quite easy too -- I typed it from memory!)
(In reply to comment #19)
> So the goal is to save work under the assumption that 'try: -b do -p all -u
> all -t all' is harder to remember than 'try: -a' causing people to think
> more?

The goal is indeed to get people to think about what they need.  Also, for folks who choose to use http://people.mozilla.org/~lsblakk/trychooser/ to build their request, it's already been updated to reflect the new world without try: -a in it.
also, the default is still to not have talos runs on every try push - and so to ask for '-t all' one hopefully intends to use/examine those results.
Comment on attachment 538033 [details] [diff] [review]
adding a couple of tests to make sure try_mandatory.hook does what it should

http://hg.mozilla.org/hgcustom/hghooks/rev/4156bf664534
Attachment #538033 - Flags: checked-in+
Hook is in place.
http://hg.mozilla.org/try/pushloghtml shows lots of try syntax usage (and lots of try: -b do -p all -u all -t none)
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.