1034395 - Allow overriding of default options

Status: RESOLVED FIXED

(Tree Management Graveyard :: OrangeFactor, defect)

Description

[Mark Côté [:mcote]]

Bug 1034065 eliminated orangefactor.conf.example in favour of checking in the defaults directly as orangefactor.conf to ease updates.  The original point of not checking in orangefactor.conf was to avoid accidentally checking in local options; however, a better way to do this would be to have options, say, orangefactor_local.conf, override those in orangefactor.conf.

Comment 1

[Sagar Pandey]

Mark : I am interested in it . May it be assigned to me ?

Comment 2

[Ed Morley [:emorley]]

Done :-)

Comment 3

[Sagar Pandey]

Ed : How do you suggest I should get started with this ?

Comment 4

[Ed Morley [:emorley]]

I'd clone the repo at https://hg.mozilla.org/automation/orangefactor/

And then have a look around the 'server' directory inside it. Whenever orangefactor.conf is used, we need to check for an optional orangefactor_local.conf (or similar name) and give any preferences in there priority over orangefactor.conf. In addition, I think this added complexity makes it worth breaking out the config handling into a separate file (eg config.py) which is imported by each of the scripts in server/ rather than having even more duplication everywhere than there already is.

Comment 5

[Sagar Pandey]

Ed : So you suggest that a new script config.py should be added which checks for the existence of orangefactor_local.config and reassign the variables mentioned in orangefactor.conf with the values from the previous file . Did I get it right ?

Comment 6

[Ed Morley [:emorley]]

Yeah that sounds good :-)

Comment 7

[Sagar Pandey]

Fine . I am on it then.

Comment 8

[Sagar Pandey]

Ed : As I see it the orange factor is configured in /server/handlers.py . Hence if we could just make apt changes in handlers.py , it will suffice to solve this bug .

Comment 9

[Ed Morley [:emorley]]

update_testfailures.py also uses orangefactor.conf, not just handlers.py:
https://hg.mozilla.org/automation/orangefactor/file/b4f49b1aa780/server/update_testfailures.py#l17

Comment 10

[Sagar Pandey]

Yeah , right . I think your suggestion will do the fix. I am working on it.

Comment 11

[Sagar Pandey]

Yeah , right . I think your suggestion will do the fix. I am working on it.

Comment 12

[Sagar Pandey]

Ed: I have coded the fix . Now what ?

Comment 13

[Ed Morley [:emorley]]

Hi Sagar! If you check out https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F it should explain what the next steps are. Just comment in the bug (use the needinfo flag on me to ensure I see it) if you have any questions :-)

Comment 14

[Sagar Pandey]

Ed: I have been seeing and trying to work around it since last 2-3 days . I have the following queries:
i) Will posting a file difference work (capella told me about this today on irc) ?
ii) Is cloning mozilla-central mandatory to put up a patch for orangefactor (using mq) ?
iii) will it be okay if a put up this repo on github and then create a patch using git and then convert to a mercurial compatible patch ? having issues with mercurial on my system due to using python 2.7.4. ?

Comment 15

[Ed Morley [:emorley]]

What step on the MDN page guide are you stuck on?

Note make sure you follow method #2 "Manually editing the .hgrc file" rather than #1 "Interactive Mercurial Setup Wizard".

(In reply to Sagar Pandey from comment #14)
> i) Will posting a file difference work (capella told me about this today on
> irc) ?

a patch generated using mq, |hg export| or |hg diff| are all file diffs, that's what we're after yeah :-)

> ii) Is cloning mozilla-central mandatory to put up a patch for orangefactor
> (using mq) ?

Not required.

> iii) will it be okay if a put up this repo on github and then create a patch
> using git and then convert to a mercurial compatible patch ? having issues
> with mercurial on my system due to using python 2.7.4. ?

I would really recommend not doing this, there's no need to use git.

Comment 16

[Sagar Pandey]

Okay , thanks Ed. I will submit the patch asap as per your suggestions and the instructions. I was actually stuck on implementing the "Interactive mercurial setup " thing as it required python-dev to be installed which is not compatible with the current version of the python i am using - python 2.7.4 . I am gonna go through the second method now .

Comment 17

[Sagar Pandey]

Attachment: bug1034395

Comment 18

[Sagar Pandey]

Attachment: bug-1034395-fix.patch

Comment 19

[Ed Morley [:emorley]]

Comment on attachment 8467286 [details] [diff] [review]
bug-1034395-fix.patch

This attachment doesn't include any changes?

See: https://bug1034395.bugzilla.mozilla.org/attachment.cgi?id=8467286

Comment 20

[Sagar Pandey]

I anticipated this . Ed, I followed every instruction except that I started creating the patch after all my code editing was over.I mean I had nothing to edit after creation of the patch . Is this causative reason for the patch not containing any changes ? Anyways I will try editing the code all from the scratch after creating a new patch and submit another one.

Comment 21

[Ed Morley [:emorley]]

Sounds like you just need to |hg qrefresh| to include the changes in the working directory in your patch. If you added a new file, you'll also need to |hg add| that file.

This guide is useful, if you wish to read up on mq:
http://hgbook.red-bean.com/read/managing-change-with-mercurial-queues.html

Comment 22

[Sagar Pandey]

Thanks Ed . It was really a helpful resource. I am attaching the new patch . Hope it works.

Comment 23

[Sagar Pandey]

Attachment: bug-1034395-fix.patch

Comment 24

[Ed Morley [:emorley]]

Comment on attachment 8468463 [details] [diff] [review]
bug-1034395-fix.patch

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

I see the patch now - thank you :-)

I think I'll get :mcote to review this, but for now a few things I've spotted:
* config.py contains a mixture of tabs and spaces - tabs are generally discouraged in mozilla projects - might be worth checking your editor defaults to spaces & converting those in this new file.
* In tuples, make sure you add spaces after the comma (eg "cfg.get('servers','bzapi')" -> "cfg.get('servers', 'bzapi')") to silence the pyflakes [1] warnings)

[1] https://pypi.python.org/pypi/pyflakes

::: server/config.py
@@ +16,5 @@
> +    except(ValueError, ConfigParser.NoSectionError, ConfigParser.NoOptionError):
> +        web.config.debug = False
> +    if os.path.exists('orangefactor_local.conf'):
> +        cfg_local = SafeConfigParser()
> +	cfg_lcoal.read('orangefactor_local.conf')

s/cfg_lcoal/cfg_local/

@@ +29,5 @@
> +		bzapi = cfg.get('servers','bzapi')
> +	else:
> +                es = cfg.get('servers','es')
> +		bzapi = cfg.get('servers','bzapi')
> +        if cfg_local.has_section('orangefactor'):	    

There's some whitespace at the end of this line.

@@ +59,5 @@
> +        trees = cfg.get('orangefactor','trees')
> +	trunk_trees = cfg.get('orangefactor','trunk_trees')
> +	exclude_platforms = cfg.get('orangefactor','exclude_platforms')
> +        exclude_tbpl_os = cfg.get('orangefactor','exclude_tbpl_os')
> +    final_var_list = [es,bzapi,trees,trunk,trees,exclude_platform,exclude_tbpl_os]

s/trunk,trees/trunk_trees/
and
s/exclude_platform/exclude_platforms/

::: server/handlers.py
@@ -26,5 @@
>  
> -CFG_FILE = 'orangefactor.conf'
> -abspath = os.path.dirname(__file__)
> -cfg = ConfigParser.ConfigParser()
> -cfg.read(os.path.join(abspath, CFG_FILE))

I believe the removal of os.path.join() makes the |import os| in this file redundant, since it was the last consumer.

@@ +31,3 @@
>  
>  # ES expects these to be tuples
> +exclude_platforms = tuple(['"%s"' % x.strip() for x in conf_dic['exclude_platforms'].split(',')])

s/conf_dic/conf_dict/

::: server/update_testfailures.py
@@ +8,3 @@
>  """
>  
>  import ConfigParser

This import is now unused, so can be removed :-)
(And same for the one in handlers.py)

Comment 25

[Sagar Pandey]

Ed:Thanks for pointing all these out . So should I wait for mcote review's too or i should follow up on your review and submit another patch ?

Comment 26

[Sagar Pandey]

Attachment: bug-1034395-fix.patch

I have applied edmorley's review suggestions .

Comment 27

[Mark Côté [:mcote]]

Comment on attachment 8468666 [details] [diff] [review]
bug-1034395-fix.patch

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

Hi, thanks for the patch!  This is a good start, but there's too much code duplication in here.  In fact, the ConfigParser module gives a very simple way to load defaults and override them with local config.  See https://docs.python.org/2/library/configparser.html#ConfigParser.RawConfigParser.read  Using that method, you should be able to reduce the size of config.py by half, at least. :)

A couple other specific comments follow.

::: server/config.py
@@ +1,1 @@
> +import os

Need an MPL header.

@@ +72,5 @@
> +                      trees,
> +                      trunk_trees,
> +                      exclude_platforms,
> +                      exclude_tbpl_os]
> +    return final_var_list

Why not return this as a dict so you don't have to convert it in get_config()?

Comment 28

[Sagar Pandey]

Attachment: bug-1034395-fix.patch

Thanks Mark for the vital resource :) . Hope this is as you suggested.

Comment 29

[Mark Côté [:mcote]]

Comment on attachment 8470316 [details] [diff] [review]
bug-1034395-fix.patch

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

Almost there! :)  A few specific changes below, plus please add something to the README.txt file explaining that default options are in orangefactor.conf and that they can be overridden with orangefactor_local.conf.  Also please add server/orangefactor_local.conf to the .hgignore file.

::: server/config.py
@@ +7,5 @@
> +
> +def get_conf():
> +    dic = {}
> +    cfg = ConfigParser.RawConfigParser()
> +    cfg.readfp(open('orangfactor.conf'))

You misspelled "orangefactor.conf" here.  Please test all your changes before uploading a patch, even if they feel trivial.

@@ +8,5 @@
> +def get_conf():
> +    dic = {}
> +    cfg = ConfigParser.RawConfigParser()
> +    cfg.readfp(open('orangfactor.conf'))
> +    cfg.read(['~/Orangefactor_local.conf'])

Please change this to 'orangefactor_local.conf'; for our purposes, I think we would normally be keeping local config in the server directory with the default config.

Also, this doesn't actually need to be a list, since you're only reading from one file, and ConfigParser.read() understands both single strings and lists of strings.  The canonical way to use read() on one file is just with a string, so may as well change this.

@@ +12,5 @@
> +    cfg.read(['~/Orangefactor_local.conf'])
> +    try:
> +        web.config.debug = cfg.getboolean('config', 'debug')
> +    except (ValueError, ConfigParser.NoSectionError,
> +            ConfigParser.NoOptionError):

I know the old code did this, but I would actually take out ValueError from this tuple.  If someone did enter in an invalid value, possibly due to a typo, it's better to error out than silently ignore it and default to False.

Comment 30

[Sagar Pandey]

Thanks . I am totally on it mark :)

Comment 31

[Sagar Pandey]

Attachment: bug-1034395-fix.patch

Comment 32

[Mark Côté [:mcote]]

Comment on attachment 8470558 [details] [diff] [review]
bug-1034395-fix.patch

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

Great!  I have one comment that I just noticed.  It's easy to fix, so I am going to r+ this patch and will make the change when I commit your patch tomorrow--unless you want to upload a new patch before then.  Either way. :)

Thanks!  There are more Orange Factor bugs lying around, if you'd like to help out more. :)  Ed Morley could direct you to the ones that he thinks are more important.

::: server/handlers.py
@@ -26,5 @@
>  
> -CFG_FILE = 'orangefactor.conf'
> -abspath = os.path.dirname(__file__)
> -cfg = ConfigParser.ConfigParser()
> -cfg.read(os.path.join(abspath, CFG_FILE))

I just noticed that you aren't constructing the path in this fashion in config.py.  We use the 'os.path.dirname(__file__)' trick to allow us to run woo_server.py from outside of the server directory.

Comment 33

[Sagar Pandey]

Yeah sure, go ahead, commit the patch with the apt changes :) I am in the middle of a travel anyway. I will be more happy to help with more orangefactor bugs . Thank you mark and Ed for all the guidance :)

Comment 34

[Mark Côté [:mcote]]

Thanks again, and no problem!

http://hg.mozilla.org/automation/orangefactor/rev/aea03f25712e

Comment 35

[Sagar Pandey]

Thanks a lot Edmorley and Mark. :)

Comment 36

[Ed Morley [:emorley]]

Deployed:

[[e-mail]3 orangefactor]$ hg inc
comparing with http://hg.mozilla.org/automation/orangefactor/
searching for changes
changeset:   238:aea03f25712e
tag:         tip
user:        Sagar Pandey <[e-mail]>
date:        Mon Aug 11 11:57:52 2014 -0400
summary:     Bug 1034395 - Allow overriding of default options. r=mcote

[[e-mail]3 orangefactor]$ hg pull -u
pulling from http://hg.mozilla.org/automation/orangefactor/
searching for changes
adding changesets
adding manifests
adding file changes
added 1 changesets with 5 changes to 5 files
5 files updated, 0 files merged, 0 files removed, 0 files unresolved
[[e-mail]3 orangefactor]$ logout
[[e-mail]3 ~]# /etc/init.d/orangefactor stop; /etc/init.d/orangefactor start
stopping orangefactor                                      [  OK  ]
starting orangefactorspawn-fcgi: child spawned successfully: PID: 16531
                                                           [  OK  ]

Comment 37

[Sagar Pandey]

Thanks Ed for all the help and guidance.

Comment 38

[Ed Morley [:emorley]]

You're welcome :-)