Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix!: only add target_tasks_method filter if no other filters were sp… #600

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

ahal
Copy link
Collaborator

@ahal ahal commented Oct 24, 2024

…ecified

BREAKING CHANGE: The target_tasks_method is now only implicitly added if no other filters exist.

This is needed to fix a bug in Gecko's ./mach try infrastructure, however the change makes sense on its own. Currently there's no way to forego target_tasks_method, and this allows us to do that.

Furthermore, there are no other built-in filters, so it's unlikely that this will impact any existing consumers.

@ahal ahal added the BREAKING CHANGE Backwards incompatible request that will require major version bump label Oct 24, 2024
@ahal ahal self-assigned this Oct 24, 2024
@ahal ahal force-pushed the push-xzoysroxpwuq branch from 6f53752 to d900d21 Compare October 24, 2024 18:38
…ecified

BREAKING CHANGE: The target_tasks_method is now only implicitly added if
no other filters exist.

This is needed to fix a bug in Gecko's `./mach try` infrastructure,
however the change makes sense on its own. Currently there's no way to
forego target_tasks_method, and this allows us to do that.

Furthermore, there are no other built-in filters, so it's unlikely that
this will impact any existing consumers.
@ahal ahal force-pushed the push-xzoysroxpwuq branch from d900d21 to dc48fef Compare October 24, 2024 18:39
@ahal ahal marked this pull request as ready for review October 24, 2024 18:40
@ahal ahal requested review from a team and gabrielBusta October 24, 2024 18:40
Copy link
Contributor

@bhearsum bhearsum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm r+'ing this on the assumption that this doesn't enable a mode of operation where target tasks are conceptually gone. If it does, I want to understand this a bit more first.

@ahal
Copy link
Collaborator Author

ahal commented Oct 25, 2024

Yes it does make it possible to by-pass the target_tasks_method filters. I'll give a brief explanation of why I need this in Gecko here, but it might be easier to explain over zoom.

In bug 1925007 I discovered that it was basically impossible to select the set of tasks that a cron task selects with ./mach try. This was immediately painful for Joel and I as we are working on the os-integration tests. But obviously would be a nice ability to have in general as well.

The reason it's not possible to select the same set of tasks, is because we override the target_tasks_method in mach try here and here.

We do this because this because usually we don't want to use whatever the normal target_tasks_method is for the project. E.g, we don't want to do the same thing as we would on a normal autoland push, we want to schedule the tasks that are listed in try_task_config.json instead.

However, in some cases we do want to use the original target_tasks_method. Cron decision tasks are a great example. We want to select from the same set of tasks that the cron task selects, however because we've overwritten target_tasks_method, that information is lost and it is no longer possible.

There's probably other ways of solving this, but my solution is here:
https://phabricator.services.mozilla.com/D226554

Essentially it converts the special "tryselect" target_tasks_methods into filters. Filters are pretty much identical to target_task_methods, and it looks like whoever added them was intending for it to replace target_tasks entirely. Tbh, up until today I was thinking we should just remove the concept of filters (because I don't really see the benefit in migrating at this point).. But this seems to be a valid use case of them?

If you have other ideas to solve the issue, I'm open to them!

@ahal
Copy link
Collaborator Author

ahal commented Oct 25, 2024

We discussed further on zoom and Ben has no concerns with landing this.

@ahal ahal merged commit 0f46df6 into taskcluster:main Oct 25, 2024
15 checks passed
@ahal ahal deleted the push-xzoysroxpwuq branch October 25, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING CHANGE Backwards incompatible request that will require major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants