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.

Sign up for GitHub

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

Jump to bottom

Create a release promotion action #5501

Merged
merged 4 commits into from May 12, 2023
Merged

Create a release promotion action #5501

merged 4 commits into from May 12, 2023

Conversation

ahal
Copy link
Member

@ahal ahal commented Apr 24, 2023 •
edited

Pull Request checklist

  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGES_UNRELEASED.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

@ahal ahal added the WIP Work in progress label Apr 24, 2023
@ahal ahal self-assigned this Apr 24, 2023
@ahal ahal force-pushed the relpro branch 2 times, most recently from 930f6a7 to c4c66e1 Compare May 9, 2023 18:53
@ahal ahal requested review from bendk, a team and hneiva May 9, 2023 18:54
@ahal ahal removed the WIP Work in progress label May 9, 2023
hneiva
hneiva reviewed May 9, 2023
View reviewed changes
taskcluster/app_services_taskgraph/transforms/deps_complete.py
@@ -5,6 +5,7 @@
from taskgraph.transforms.base import TransformSequence

transforms = TransformSequence()
alerts = TransformSequence()
Copy link

Choose a reason for hiding this comment

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

is there an implicit logic for a transform wrappers if the wrapper is called alerts? I don't see where this gets used

Copy link
Member Author

Choose a reason for hiding this comment

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

It's used e.g, here: https://github.com/mozilla/application-services/pull/5501/files#diff-3fd946aa80e283c20e3fc25009310bc8360f75aa76a03c9a34bbce67e67d7c18R9

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I could have pulled that out into a new transforms file, but I was feeling a bit lazy. Not opposed to doing so if folks want

bendk
bendk previously approved these changes May 9, 2023
View reviewed changes
Copy link
Contributor

@bendk bendk left a comment

Choose a reason for hiding this comment

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

I don't have enough knowledge to comment on the release_promotion.py script, but the other changes look good to me.

I look forward to iterating on this, I think this could improve some of the code I recently added for nightlies.

jcristau
jcristau reviewed May 10, 2023
View reviewed changes
Copy link
Contributor

@jcristau jcristau left a comment

Choose a reason for hiding this comment

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

One thing that confuses me while reviewing this is that taskgraph target-graph -p taskcluster/test/params/release-branch-push.yml includes signing and beetmover tasks, apparently related to the branch-build stuff which I don't understand, but seems wrong.

taskcluster/app_services_taskgraph/actions/release_promotion.py Outdated
parameters = get_artifact(previous_graph_ids[0], "public/parameters.yml")

# Download and combine full task graphs from each of the previous_graph_ids.
# Sometimes previous relpro action tasks will add tasks, like partials,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not sure partials is the right example here :)

Copy link
Member Author

Choose a reason for hiding this comment

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

This was definitely not copy/pasted from elsewhere...

taskcluster/app_services_taskgraph/actions/release_promotion.py Outdated
"type": "string",
"description": "The flavor of release promotion to perform.",
"default": "promote",
"enum": sorted(graph_config["release-promotion"]["flavors"].keys()),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: no need for .keys()?

@bendk
Copy link
Contributor

bendk commented May 10, 2023

One thing that confuses me while reviewing this is that taskgraph target-graph -p taskcluster/test/params/release-branch-push.yml includes signing and beetmover tasks, apparently related to the branch-build stuff which I don't understand, but seems wrong.

That seems wrong to me as well, and I don't think branch-build should come into play here. I think this should either be fixed in this PR or I can fix it in a follow-up.

bendk
bendk reviewed May 10, 2023
View reviewed changes
taskcluster/app_services_taskgraph/target_tasks.py
)
return filter_release_promotion(
full_task_graph, filtered_for_candidates, shipping_phase="ship"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that we'll run the promote tasks again in the ship phase or will they be cached somehow?

Copy link
Contributor

Choose a reason for hiding this comment

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

They'll typically be optimized out thanks to existing_tasks; this allows e.g. skipping promote and running ship directly without tasks going missing.

@ahal
Copy link
Member Author

ahal commented May 10, 2023

One thing that confuses me while reviewing this is that taskgraph target-graph -p taskcluster/test/params/release-branch-push.yml includes signing and beetmover tasks, apparently related to the branch-build stuff which I don't understand, but seems wrong.

Good catch, I'll investigate.. This is unrelated to the current PR though as I can reproduce on main as well. That reminds me that I should add some relpro params files too.

@mergify mergify bot dismissed bendk’s stale review May 10, 2023 15:20

The pull request has been modified, dismissing previous reviews.

@ahal ahal marked this pull request as ready for review May 11, 2023 14:23
@ahal ahal requested a review from jcristau May 11, 2023 14:27
@ahal ahal requested a review from hneiva May 12, 2023 13:33
@ahal ahal added this pull request to the merge queue May 12, 2023
Merged via the queue into mozilla:main with commit 08e9af4 May 12, 2023
17 checks passed
@ahal ahal deleted the relpro branch May 12, 2023 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bendk bendk bendk left review comments

@jcristau jcristau jcristau approved these changes

@hneiva hneiva Awaiting requested review from hneiva

Assignees

@ahal ahal

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ahal @bendk @hneiva @jcristau