Last Comment Bug 779294 - should allow for multiple config files
: should allow for multiple config files
Product: Release Engineering
Classification: Other
Component: General Automation (show other bugs)
: other
: x86 Mac OS X
: P4 normal (vote)
: ---
Assigned To: Aki Sasaki [:aki]
: Chris AtLee [:catlee]
Depends on:
Blocks: 814717 vcs-sync 840724
  Show dependency treegraph
Reported: 2012-07-31 13:53 PDT by Aki Sasaki [:aki]
Modified: 2013-08-12 21:54 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

first pass (14.92 KB, patch)
2013-02-11 23:53 PST, Aki Sasaki [:aki]
no flags Details | Diff | Splinter Review
with tests, fix (17.17 KB, patch)
2013-03-01 21:43 PST, Aki Sasaki [:aki]
rail: review+
aki: checked‑in+
Details | Diff | Splinter Review

Description Aki Sasaki [:aki] 2012-07-31 13:53:18 PDT
This would change --config-file to an extend or append here:

Also, here we'd need to parse the config files as a list, in order:

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 2012-09-25 10:26:54 PDT
-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 [:aki] 2013-01-30 13:32:00 PST
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 [:aki] 2013-02-11 23:53:20 PST
Created attachment 712825 [details] [diff] [review]
first pass

I'd like some tests around multiple config files.
Comment 4 Aki Sasaki [:aki] 2013-02-12 11:02:13 PST
(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 [:aki] 2013-03-01 21:43:41 PST
Created attachment 720225 [details] [diff] [review]
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] ⌚️ET 2013-03-04 05:39:52 PST
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 [:aki] 2013-03-04 11:04:05 PST
Yeah, sorry for not mentioning that.
Thanks for the review!
Comment 8 Aki Sasaki [:aki] 2013-03-04 11:05:47 PST
Comment on attachment 720225 [details] [diff] [review]
with tests, fix
Comment 9 Aki Sasaki [:aki] 2013-03-04 11:31:58 PST
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 [:aki] 2013-03-04 14:56:03 PST
Merged to production after seeing a green Cedar test.

Note You need to log in before you can comment on or make changes to this bug.