779294 - BaseConfig.py should allow for multiple config files

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P4)

Description

[Aki Sasaki (not active)]

This would change --config-file to an extend or append here:
http://hg.mozilla.org/build/mozharness/file/e93ae357f521/mozharness/base/config.py#l197

Also, here we'd need to parse the config files as a list, in order:
http://hg.mozilla.org/build/mozharness/file/e93ae357f521/mozharness/base/config.py#l329

This adds enough complexity that it would be good to have unit tests written to test this.

While we're at it, maybe --config-file/--cfg is such a high-usage option that it makes a sense to create a single-char option for it like -c or -f.

Comment 1

[Jeff Hammel]

-c is fairly common for specifying configuration files.  I'm not sure if the leftover args are used at all in any existing mozharness script.  If not, those are options too.

Comment 2

[Aki Sasaki (not active)]

Bonus points:

* allow for optional config files.

right now if the config file doesn't exist, we'll hit an exception.
maybe -C required_config_file -c optional_config_file ?

I don't know how to preserve order here in OptParse if we do this, however.

* load configs from a url.

If we solve this bug without one or both of these, we can file new bugs for them.

Comment 3

[Aki Sasaki (not active)]

Attachment: first pass

I'd like some tests around multiple config files.

Comment 4

[Aki Sasaki (not active)]

(In reply to Jeff Hammel [:jhammel] from comment #1)
> I'm not sure if the
> leftover args are used at all in any existing mozharness script.  If not,
> those are options too.

Not sure if I want the leftover args to be config files or actions.

Comment 5

[Aki Sasaki (not active)]

Attachment: with tests, fix

Looks like this works.

* pep8 for mozharness.base.config
* a couple new test config files
* removed the unused --action and --only-ACTION options (now just use --ACTION or --no-ACTION or --add-action ACTION)
* tests for multi config files

Comment 6

[Rail Aliiev [:rail]]

Comment on attachment 720225 [details] [diff] [review]
with tests, fix

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

I was a little bit confused by the "extend" action of ExtendOption, because I was expecting "append" instead (as list's methods). Then I read the source and found that it splits parameters by coma, so accepts multiple parameters, what makes sense.

Comment 7

[Aki Sasaki (not active)]

Yeah, sorry for not mentioning that.
Thanks for the review!

Comment 8

[Aki Sasaki (not active)]

Comment on attachment 720225 [details] [diff] [review]
with tests, fix

http://hg.mozilla.org/build/mozharness/rev/56f801571d30

Comment 9

[Aki Sasaki (not active)]

I'd like to see some Cedar green with this on default before merging to production.

Filed bug 847526 for comment 2.

Comment 10

[Aki Sasaki (not active)]

Merged to production after seeing a green Cedar test.