Closed Bug 1401199 Opened 2 years ago Closed 2 years ago

[tryselect] Handle parameter mismatch in task generation


(Testing :: General, enhancement)

Version 3
Not set


(firefox58 fixed)

Tracking Status
firefox58 --- fixed


(Reporter: ahal, Assigned: ahal)


(Blocks 1 open bug)



(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.
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.
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
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:

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.
Seems reasonable!  No, migrations weren't on my todo list :)
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
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.
Depends on: 1404067
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.
[APPROVED]           sounds like a great plan
Assignee: nobody → ahalberstadt
Attachment #8913369 - Attachment is obsolete: true
Comment on attachment 8913749 [details]
Bug 1401199 - [mozversioncontrol] Add property to get hash of HEAD revision,

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 on attachment 8913750 [details]
Bug 1401199 - [taskgraph] Use default parameter values when strict=False,

::: taskcluster/taskgraph/
(Diff revision 1)
> +_head_ref = None
> +
> +
> +def get_head_ref():

would @memoized help?

::: taskcluster/taskgraph/
(Diff revision 1)
> +
>  # Please keep this list sorted and in sync with taskcluster/docs/parameters.rst
> -    'base_repository',
> -    'build_date',
> -    'filters',
> +# Parameters are of the form: {name: default}
> +    'base_repository': '',
> +    '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 on attachment 8913751 [details]
Bug 1401199 - [tryselect] Pass in strict=False when generating tasks,
Attachment #8913751 - Flags: review?(dustin) → review+
Pushed by
[mozversioncontrol] Add property to get hash of HEAD revision, r=mshal
[taskgraph] Use default parameter values when strict=False, r=dustin
[tryselect] Pass in strict=False when generating tasks, r=dustin
You need to log in before you can comment on or make changes to this bug.