The default bug view has changed. See this FAQ.

BaseConfig.py should allow for multiple config files

RESOLVED FIXED

Status

Release Engineering
General Automation
P4
normal
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: aki, Assigned: aki)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozharness])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Priority: -- → P4

Comment 1

5 years ago
-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.
(Assignee)

Comment 2

4 years ago
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.
(Assignee)

Updated

4 years ago
Blocks: 814717
(Assignee)

Comment 3

4 years ago
Created attachment 712825 [details] [diff] [review]
first pass

I'd like some tests around multiple config files.
Assignee: nobody → aki
(Assignee)

Comment 4

4 years ago
(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.
Blocks: 840724
(Assignee)

Comment 5

4 years ago
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
Attachment #720225 - Flags: review?(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.
Attachment #720225 - Flags: review?(rail) → review+
(Assignee)

Comment 7

4 years ago
Yeah, sorry for not mentioning that.
Thanks for the review!
(Assignee)

Updated

4 years ago
Attachment #712825 - Attachment is obsolete: true
(Assignee)

Comment 8

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

http://hg.mozilla.org/build/mozharness/rev/56f801571d30
Attachment #720225 - Flags: checked-in+
(Assignee)

Comment 9

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

Filed bug 847526 for comment 2.
(Assignee)

Updated

4 years ago
Blocks: 799719
(Assignee)

Comment 10

4 years ago
Merged to production after seeing a green Cedar test.
Status: NEW → RESOLVED
Last Resolved: 4 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.