Bug 1343990 - add Fennec Release Graph 2. #219
Conversation
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.
Non-expert stamp
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.
In overall it looks great, just a couple of issues. Almost there!
|
||
product: "fennec" | ||
product: "mobile" |
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.
Hmmm, we don't usually use "mobile" for product, it's either stage_product or appName, see https://hg.mozilla.org/build/buildbot-configs/file/tip/mozilla/release-fennec-mozilla-release.py.template#l19. I'm not sure where it is used in the graph, but I'd check it. This also applies to the file renames.
- | ||
taskId: "{{ stableSlugId(buildername) }}" | ||
requires: | ||
- "{{ stableSlugId('publish_release_human_decision') }}" |
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.
I'd rather make this dependent on push to cdns. If push to cdns fails, we shouldn't update the bouncer aliases.
- | ||
taskId: "{{ stableSlugId(buildername) }}" | ||
requires: | ||
- "{{ stableSlugId('publish_release_human_decision') }}" |
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 same here, cdns first, then the rest
- | ||
taskId: "{{ stableSlugId(buildername) }}" | ||
requires: | ||
- "{{ stableSlugId('publish_release_human_decision') }}" |
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.
And here, cdns first
@rail Thanks again for the suggestions - I revisited my patch and was able to improve the things a bit at a second round. So here we go:
|
@@ -0,0 +1,45 @@ | |||
{% set buildername = "release-{}-{}_version_bump".format(branch, 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.
Have we changed the script to make sure we don't downgrade the version? It shouldn't be too hard.
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 script itself you mean? Am not sure yet. The builder is set to False anyway until I add the necessary configs within mozharness and buildbot-configs.
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 I merge and address this later when I deal with postrelease builders anyway or should I look into it now?
Merged! |
@rail @escapewindow @kmoir
Few observations:
Shouldn't we split the release configs into "beta" and "release" or some sort? I know we're dealing with beta only now, but it'd be worth renaming those to "beta" and copy-paste later to obtain the "release" ones.
I think it'd be worth chaining the checksum builder in the second graph. Currently, "push-to-releases" depends on it. We can either remove that dep or enable checksums within the second graph.
I filed bugs against 1343990 to mark the changes we need to make in the second graph builders to make them work. However, right now we only need the
push-to-releases
builder which works fine. The rest of them can be disabled and/or run manually.