-
Notifications
You must be signed in to change notification settings - Fork 27
Bug 1890753 - Add support for merge automation to shipit #1801
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
base: main
Are you sure you want to change the base?
Conversation
f63ec88 to
15b504b
Compare
| parameters: | ||
| - name: product | ||
| in: query | ||
| description: product_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the product here work? It probably doesn't make sense to have separate merge automation for firefox/devedition/firefox-android? [nevermind, I see we return 400 if there's no merge-behaviors defined for the passed-in product, and we only define it for firefox]
| # NOTE: This duplicates some configuration between the backend and the frontend (mainly the repo names). | ||
| # However, the frontend should never have half of the config it has in the first place and it should get moved at some point. | ||
| # See bug 1879910 | ||
| _MERGE_BEHAVIORS_PER_PRODUCT = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this live in products.yml?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with that is that I guess I slightly overloaded the product term here. I guess it's more of a product group than a product? Since it targets a specific set of repos/branches that might contain multiple products (i.e firefox, fenix) so I don't think we should put it in products.yml? At least not without introducing groups in there to indicate products living in the same repo, but that's way out of scope IMO
This allows displaying a message to a user after they get redirected from filling a form, explaining what changed.
This is slightly annoying because I would really like to avoid having configuration on the frontend side but this is the first feature to require that which means we have to figure out how to do it per environment. The frontend config duplicates the entire thing in 3 different files, instead here I chose to go with the taskgraph looking way of using `by-env`. This is not perfect, it's very verbose but it works. As the comment says, this duplicates some values (repo URLs, version file path...) with the frontend configuration but said config shouldn't exist in the first place so I'm fine with that for now. Do note that this config is a placeholder for now, used for testing. It's missing a lot of merge behaviors and is using the wrong values for beta-to-release so we can local test what happens with always-target-tip
`TaskStatus` here maps to taskcluster statuses, minus the difference between pending/scheduled and failure/exception as those have the same high level meaning. Each merge automation is made up of 4 inputs. - The product it targets (this might be a bit of a misnomer, because it's running on repositories, not shipit products. But then firefox's merge automation is running on multiple repositories so I decided to keep it simple and keep it named product, have one for firefox). Right now the goal is to support firefox only but this is future proofing because I know that firefox-ios would also benefit from merge automation. - The behavior to run. This is the name of the action to run, i.e `main-to-beta`. - The revision to run it from. Technically for firefox this doesn't really matter because AFAIK it will always merge the tip of the branch but this is where the action will be taken from and that is important for try. - And finally whether that is a dry run or not. Then we have a few properties regarding the status of it. When it gets created, its task ID is null, status is pending. Then as it starts, we need to record the task id of the action task we just created and update the status based on the status of both the action task and the resulting group. The rest of the fields is caching, to avoid querying HG again since the data is static and will never change (and querying hg is **slow**), and to avoid issues if the configuration ever changes.
This gives the LDAP group allowed to create releases for a given product the rights to run merge automation for said product.
This is a simple menu just showing a button to list all merge automations and create new ones. I did not bother to make a "per product" menu as for releases as firefox is the only supported one right now.
The one part in here I'm unhappy with is the completion mechanism for merge automations because it requires someone querying it to update it which goes against good practice. I did this to avoid having to change task groups to add a task similar to `mark-as-shipped` to merge automation and having to uplift it in every tree. In practice, that status is only ever used from the shipit UI so it'll never fall out of sync unless the tasks expire before someone visits the merge automation page again which is never going to happen. The other alternative would be to hook the worker that currently handles product details to poll taskcluster and update the status instead.
This was a bit of a pain because I'm not super familiar with react and I'm **bad** at UI. The page to create automation is very similar to the release creation one but for the listing I wanted to try something a bit more user friendly so it's showing the task state from taskcluster for both the action task and the resulting group.
While it would help with caching to do it from the backend as before, the gains are not that significant as one would rarely get data for the same commit multiple times and the drawbacks of making the shipit API slower and hiding failures do to hg behind it are not worth it.
15b504b to
d1114bf
Compare
This lets us keep a record of cancelled automations which matches what releases do
…k_id As well as pagination, this now re raises the original exception instead of a bare Exception to avoid eating context of said exception so consumers can actually use it.
Instead of relying on the side effect of the GET request, make merge automation require a call to mark them as complete to match what releases do. This also adds a merge-automation-id parameter to the merge automation action trigger so that they can actually call that new route
d1114bf to
1cd3c22
Compare
|
With mozilla-releng/scriptworker-scripts#1312 and https://treeherder.mozilla.org/jobs?repo=try&revision=65f49a8db3368861f7c1348133cd0976aa874da5 (which didn't run correctly because of missing scopes and wouldn't work anyway because I don't want to deploy this PR to dev before we're sure won't have a schema change) we should have a proper mark-as-merged action working. |
This will allow relman to run merge automation without having to copy paste payloads manually in treeherder.
Right now this only supports firefox but it should be fairly easy to add other products.
The configuration is very minimal and isn't valid, it is that way to help with testing at the moment and it should be changed before we consider merging this.
Screenshots
Creating a new merge automation
From try:
From mozilla-beta (no choice of target commit, will always pick the tip)
Merge automation list
After creation:
After start:
Done:
Cancellation popup: