Closed
Bug 1401199
Opened 7 years ago
Closed 7 years ago
[tryselect] Handle parameter mismatch in task generation
Categories
(Testing :: General, enhancement)
Tracking
(firefox58 fixed)
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: ahal, Assigned: ahal)
References
Details
Attachments
(3 files, 1 obsolete file)
This bug might belong under Taskcluster::Task Configuration depending on the approach we take.
Currently taskcluster task generation downloads the latest parameters.yml from mozilla-central by default. This means if you push from a revision that has a different set of parameters from central tip (which gets more likely the further behind or ahead of central you are), you'll get an error about either a missing parameter, or an extra parameter. This can be fixed by either rebasing on central or passing in e.g --parameters project=mozilla-beta, or --parameters task-id=<task-id>.
At the very least we should be printing a more informative error message here explaining what to do. But it would be better if we could help the user out. There might be some way to automatically find the decision task-id based on a revision, in which case we could detect the latest public revision the user is based on and grab its parameters.yml.
Alternatively we could not run this "check" when using |mach try|. Though this could mean task generation will simply fail later on.
Assignee | ||
Comment 1•7 years ago
|
||
It's worth mentioning that this problem exists for the |mach taskgraph| subcommands as well, though those aren't used much by the wider developer audience so it's not as big of a deal if it breaks from time to time.
Comment 2•7 years ago
|
||
I chose the "be strict in what you accept" approach to parameters, but you're right -- it causes pain. I would like to keep the check, but we can probably file off some of the pointier edges.
A few ideas:
- version the parameters, with migration functions (most will just add default values or rename things)
- allow default values and print warnings when they are used
Assignee | ||
Comment 3•7 years ago
|
||
I kind of like that things are strict, versions+migrations seem like they will be a lot of work (though if it's something you were thinking of doing anyway, then yeah it would help).
I think we could add a route like this to the decision tasks:
index.gecko.v2.revision.${push.revision}.decision
Then we just need to query vcs for the latest public revision. This route purposefully excludes ${repository.project} as that's something that's hard to determine from a local vcs, so just needing the revision should work no matter what project the user is based on.
Because querying vcs can be slow(ish), and there's a chance that the route with the revision doesn't exist, I'd recommend doing this:
1. Grab parameters.yml from latest central as usual.
2. If the check fails, try to grab parameters.yml based on latest public revision.
3. If anything goes wrong, print an informative error message and force user to specify --parameters manually.
Comment 4•7 years ago
|
||
Seems reasonable! No, migrations weren't on my todo list :)
Assignee | ||
Comment 5•7 years ago
|
||
This dependent bug adds an "upstream" parameter to mozversioncontrol that we'll be able to use to find the latest public revision on git, so let's block on that first.
Depends on: 1401309
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
Latest patch is a WIP. It doesn't work yet because apparently we need to add a scope to access the added route to all repositories.
In the meantime, I think I'm going to file a bug to get just the better error message landed. That way if this happens again, at least it will be a little less confusing.
Assignee | ||
Comment 8•7 years ago
|
||
Given the scope issue and that the current proposal still has the potential to fail, I think it'll be better to look into providing default values for some or all parameters. I realize this was suggested in comment 2, I think I glossed over it a bit too quickly.
I think a good way to implement this will be to have a `strict=True` argument to load_parameters_file(). This way it'll still be strict when called from e.g the decision task, but things like |mach try fuzzy| can pass in False. We can still grab the mozilla-central parameters.yml as a starting point. When strict==False, we can simply ignore extra parameters, and fill in default values for missing parameters.
If a missing parameter has no default, then it will raise and we print the helpful error message that got added by bug 1404067.
Comment 9•7 years ago
|
||
[APPROVED] sounds like a great plan
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8913369 -
Attachment is obsolete: true
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8913749 [details]
Bug 1401199 - [mozversioncontrol] Add property to get hash of HEAD revision,
https://reviewboard.mozilla.org/r/185138/#review190188
LGTM. I was surprised we don't have this already, but it looks like we have a bunch of scripts that do their own revision detection. Would be nice if they can all use this.
Attachment #8913749 -
Flags: review?(mshal) → review+
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8913750 [details]
Bug 1401199 - [taskgraph] Use default parameter values when strict=False,
https://reviewboard.mozilla.org/r/185140/#review190226
::: taskcluster/taskgraph/parameters.py:27
(Diff revision 1)
>
>
> +_head_ref = None
> +
> +
> +def get_head_ref():
would @memoized help?
::: taskcluster/taskgraph/parameters.py:41
(Diff revision 1)
> +
> # Please keep this list sorted and in sync with taskcluster/docs/parameters.rst
> -PARAMETER_NAMES = set([
> - 'base_repository',
> - 'build_date',
> - 'filters',
> +# Parameters are of the form: {name: default}
> +PARAMETERS = {
> + 'base_repository': 'https://hg.mozilla.org/mozilla-unified',
> + 'build_date': lambda: int(time.time()),
This could probably safely capture time.time() at module load, but what you've got here is pretty fancy :)
Attachment #8913750 -
Flags: review?(dustin) → review+
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8913751 [details]
Bug 1401199 - [tryselect] Pass in strict=False when generating tasks,
https://reviewboard.mozilla.org/r/185142/#review190228
Attachment #8913751 -
Flags: review?(dustin) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e7a7ec82f82d
[mozversioncontrol] Add property to get hash of HEAD revision, r=mshal
https://hg.mozilla.org/integration/autoland/rev/a91196fe2eb3
[taskgraph] Use default parameter values when strict=False, r=dustin
https://hg.mozilla.org/integration/autoland/rev/f787be169098
[tryselect] Pass in strict=False when generating tasks, r=dustin
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e7a7ec82f82d
https://hg.mozilla.org/mozilla-central/rev/a91196fe2eb3
https://hg.mozilla.org/mozilla-central/rev/f787be169098
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•