1111310 - Allow to configure mozregression with config file(s)

Status: RESOLVED FIXED

(Testing :: mozregression, defect)

Description

[Julien Pagès (:parkouss)]

This is something that could be useful in mozregression, the ability to be configured by config files. For good integration with argparse, I found this:

http://blog.vwelch.com/2011/04/combining-configparser-and-argparse.html

that seems pretty good to me.

Here is what I am thinking:

We could check for the existence of a configuration file by default, say ~/.mozregression/config. If it exists, we read it and take the values inside as defaults.

the ".mozregression" directory will allow us to put other things than just the config file, for example a default
http_cache_dir (see bug 1019528):

~/.mozregression/config
~/.mozregression/http_cache/

I think that could be a good first (or second) bug. I may mentor it if that's OK for you; what do you think ?

Comment 1

[Julien Pagès (:parkouss)]

Hey Jonathan, are you interested to work on this ?

Comment 2

[jonathan.pigree]

Hello. of course I am. I will keep you up to date on my progress.

Comment 3

[Julien Pagès (:parkouss)]

Nice ! Please ask me if you need more information about it.

Comment 4

[jonathan.pigree]

Okay. Sorry for the delay. Finally, I have a little free time to spare. I just re-read the issue. I will work on it this week.

Comment 5

[Julien Pagès (:parkouss)]

Great. :) Do not hesitate to ask if you have any question.

Comment 6

[jonathan.pigree]

Attachment: configuration file prototype

It may look trivial but just in case.

Comment 7

[jonathan.pigree]

Okay. The article linked in the issue provide an easy way to implement this.

I attached a configuration file sample to know what you think of it.

I did not showed the incompatibilities between options. What do we want ?

To exit with an error when two incompatible options are specified in a configuration file ? I think it is the safer way.

Comment 8

[Julien Pagès (:parkouss)]

Comment on attachment 8554145 [details]
configuration file prototype

(In reply to jonathan.pigree from comment #7)
> Okay. The article linked in the issue provide an easy way to implement this.
> 
> I attached a configuration file sample to know what you think of it.

Yes, that is the kind of configuration file that we would like.

> I did not showed the incompatibilities between options. What do we want ?
> 
> To exit with an error when two incompatible options are specified in a
> configuration file ? I think it is the safer way.

I suppose that you are talking of options like --good and --good release (both
options can't be used at the same time, this is meaningless). Well I think that
would be handled automatically with the rest of the mozregression code.

The idea here is just to be able to a user to specify some options he uses everytime
(providing some user specific defaults differents than the ones hardcoded).
I don't think this is the case of --good, for example, but I'm thinking that if
someone always use command like:

mozregression --persist /tmp/mozreg ...

It could be good to add the ability for him to write a conf file in ~/.mozregression/config.ini

with a content like:

[default]
persist = /tmp/mozreg

So he would then just have to type:

mozregression ...

because the config file would have been read and used to define default values. Though it
is still possible for him to override it by passing a --persist command line option.

Another use case is a user that often test fennec, and not firefox (which is the default).
He could write the config like this:

[default]
app = fennec

So he does not have to specify each time the command parameter --app fennec.

Options like --good, --bad are not really useful here, as it makes no sense to provide defaults
for them.

Thanks for tackling this bug. :)

Comment 9

[jonathan.pigree]

Okay. The work here is to list all those options(+ their default values) we want to be able to preconfigure.

From what I see, it is: 
--inbound
--find-fix
--addon
--profile
--app
--repo
--bits
--command
--persist
--http-cache-dir
--http-timeout

I think I understand it quite clearly now. Thanks. I will have a pull request ready tomorrow hopefully.

Comment 10

[Julien Pagès (:parkouss)]

As we discussed with Jonathan more about it to find a good way of implementing this, it appears to me that a configurable config file path may be not suited here (it may just add some complexity for no use).

Maybe we could just hardcode '~/.mozregression/config.ini' as the only config file that mozregression can read (if present) and that would be sufficient (I'm really sorry Jonathan for the work you have already done...).

@Will
Do you have some preferences on this ? I know that I NI you a LOT these times, sorry for that... But I don't want to take in mozreg things that you would not want. :)

Comment 11

[William Lachance (:wlach)]

Works for me. Maybe just ~/.mozregression.cfg ?

Comment 12

[Julien Pagès (:parkouss)]

Sold. Less complexity around.

Comment 13

[jonathan.pigree]

Okay. I think I have got something. I still need to update the unitary tests though.

Comment 14

[jonathan.pigree]

Attachment: 1111310.patch

Implementation of the feature

Comment 15

[jonathan.pigree]

Also, still need to handle case when values in the conf file contains "". The parser keeps those "" inside the strings its extract from the file.

Comment 16

[Julien Pagès (:parkouss)]

Comment on attachment 8558803 [details] [diff] [review]
1111310.patch

Looks good to me !

Some remarks:

+        print("%s not found => no custom default values" % DEFAULT_CONF_FNAME)
Not sure this is useful - let's just print nothing when no config file is found.

-                        action='append',
+                        action="append",
This is not needed here.

As a general note, I first wondered why you did not use parser.set_defaults (https://docs.python.org/3/library/argparse.html#argparse.ArgumentParser.set_defaults), but I see that it does not change the type so we would have to handle int/float custom values anyway. So your approach works for me.

If you have some time, could you write tests for this ? And send it back here with a pull request, so our build check will trigger to ensure everything's good.

Thanks Jonathan!

Comment 17

[jonathan.pigree]

Great !!! I will do the unitary tests then submit a pull request. I have a question regarding the pep8 tool checker we use on mozregression. Is it very strict or did we configure it to ignore minor things ?

I began to fix things on "main.py" but I still had a little effort to put on it in order to completly clear all warnings.

Comment 18

[Julien Pagès (:parkouss)]

(In reply to jonathan.pigree from comment #17)
> Great !!! I will do the unitary tests then submit a pull request. I have a
> question regarding the pep8 tool checker we use on mozregression. Is it very
> strict or did we configure it to ignore minor things ?
> 
> I began to fix things on "main.py" but I still had a little effort to put on
> it in order to completly clear all warnings.

The mozregression code base is already pep8 compliant on master (see https://github.com/mozilla/mozregression/commit/5c1ba2e77eb6066b65e20fbbdda7afa0afec5826). The pep8 tool checker is "pep8", you can run the same commands as does the travis ci build:

pip install pep8
pep8 mozregression tests setup.py

Comment 19

[jonathan.pigree]

Okay. You merged it into "mozilla/master" two days ago. Perhaps I was not totally up to date the last time I worked on mozregression. That's why I add so many warnings from the pep8 tool. I will check the code again after update.

Comment 20

[jonathan.pigree]

Attachment: pull request candidate to resolve issue

Comment 21

[jonathan.pigree]

Argh. The travis check failed on the test function I wrote. I am fixing it right now.

Comment 22

[jonathan.pigree]

Okay. I have a strange behavior. The Appveyor tests won't pass but they pass on my dev machine. Even when cloning my repo from my fork on github. I don't know. As if, the config temporary file as not be created. Is there a way to print more output from the tool ?

Comment 23

[Julien Pagès (:parkouss)]

Hi Jonathan,

I suspect that the use of tempfile.NamedTemporaryFile() may behave differently on windows/linux. I noted some things on the PR, tell me if that helps.

Comment 24

[jonathan.pigree]

Okay. I understand.I didn't know AppVeyor pass tests on windows. It may be what you say. I will use the code you gave me. Thanks.

I will correct my pull request and see if it helps.

Comment 25

[Julien Pagès (:parkouss)]

Comment on attachment 8564782 [details] [review]
pull request candidate to resolve issue

OK, I remove the review flag here because I got spammed by bugzilla. :)

Jonathan, feel free to ask for another review once you completed the issue here.

Comment 26

[jonathan.pigree]

No problem. I have corrected my pull request.

Comment 27

[Julien Pagès (:parkouss)]

Comment on attachment 8564782 [details] [review]
pull request candidate to resolve issue

Yeah, this looks good to me, thanks!

There is some nits I would like you to address, then rebase your changes (be careful, there is my last commit in you PR ?) and ping me here so I can merge this in!

Comment 28

[jonathan.pigree]

Great. It seems rather easy. I will finish it tonight.
Each time I work on mozregression I do a "git pull --rebase mozilla master" then a "python setup.py develop" so I think I am pretty safe.

Thanks for the review.

Comment 29

[jonathan.pigree]

All right. It seems okay.

Comment 30

[Julien Pagès (:parkouss)]

Awesome! I merged this in, thanks. :)