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

Notify (downstream) projects about required migrations #869

Closed
fweikert opened this issue Nov 7, 2019 · 16 comments · Fixed by #885 or #913
Closed

Notify (downstream) projects about required migrations #869

fweikert opened this issue Nov 7, 2019 · 16 comments · Fixed by #885 or #913
Assignees

Comments

@fweikert
Copy link
Member

fweikert commented Nov 7, 2019

Currently we have two ways of testing upcoming incompatible flags on CI:

  1. A downstream pipeline: https://buildkite.com/bazel/bazelisk-plus-incompatible-flags
  2. Individual pipelines with "bazelisk --migrate": https://github.com/bazelbuild/continuous-integration/blob/master/buildkite/README.md#testing-with-incompatible-flags

However, in both cases the project owners never get notified about any breakages. As a result, the burden is still on the Bazel team.

Consequently, we should implement a notification mechanism that informs project owners about required migrations. Buildkite already has some support for notifications: https://buildkite.com/blog/goodbye-notifications-hello-services

@fweikert fweikert self-assigned this Nov 7, 2019
@fweikert
Copy link
Member Author

fweikert commented Nov 7, 2019

cc @laurentlb @hlopko

@jin
Copy link
Member

jin commented Nov 7, 2019

Looking forward to this.

@hlopko
Copy link
Member

hlopko commented Nov 8, 2019

This would be great! It would allow rules maintainers to migrate the rules as quickly as possible, leaving more time for leaf projects to catch up. Many maintainers are actually happy to migrate their projects, just that they don't proactively check the bazelisk pipeline often. We've heard Tensorflow or upb explicitly ask for something like this. I'm sure protobuf folks would also appreciate these notifications.

And hopefully, Bazel Green team won't need to go to each project that isn't migrated yet, create an issue by hand, and remove the project from the downstream pipeline after each incompatible flag flip.

@laurentlb
Copy link
Contributor

Any ETA for this?

Any simple solution would be helpful, even if it sends a weekly summary (with all the projects) to a single email address (we would then tell the owners to subscribe to it).

@fweikert
Copy link
Member Author

I can start working on this on Tuesday next week

@aiuto
Copy link
Contributor

aiuto commented Nov 15, 2019

cc: @aiuto

@philwo
Copy link
Member

philwo commented Nov 15, 2019

@fweikert Thank you!! :) I'll put it on our whiteboard.

When you're ready on Tuesday, before you start coding, could you outline a quick plan here how you would do it? I would like to make sure that we all agree on how the first version should look like.

@fweikert
Copy link
Member Author

fweikert commented Nov 25, 2019

Didn't have time for this issue last week, so here we go:

I propose to add a new flag (--notify_...) to bazelci.py, which is simply forwarded to aggregate_incompatible_flags_test_result.py in order to not clutter bazelci.py any more.

If the flag is passed to aggregate_incompatible_flags_test_result, this script does the following:
For every project that needs migration, it reads the GitHub repo from bazelci.py (it already has access to it) and checks if there is already an open issue. If that's the case, it may post a comment (optional). If not, it will create a new one. The issue contains the Bazel version, all offending flags and a link to a Buildkite build.

I propose to have at most one issue per Bazel release, i.e. the script will file a new issue for 3.0 even if there is already one for 2.0.

Potential blockers:
Can we simply create issues in other projects? We have access to a GitHub API token.

Potential improvements:
Add the logic to a new Python file, which requires some more changes to bazelci.py and Buildkite.

What about email support?
In theory we could add an "email" field to the DOWNSTREAM dict in bazelci.py, which could be used to send an email instead of filing an issue. However, this requires some more bookkeeping since we don't want to spam our users.

@philwo
Copy link
Member

philwo commented Nov 25, 2019

I propose to have at most one issue per Bazel release

Alternatively maybe one issue per flag - what do project owners think would work better for them? Although that might be too spammy and we could even run into rate limiting for creating too many issues...

@hlopko
Copy link
Member

hlopko commented Dec 14, 2019

I personally would prefer one issue per flag, so issues can be assigned to multiple people and their progres tracked independently.

fweikert added a commit to fweikert/continuous-integration that referenced this issue Dec 14, 2019
bazelci.py now stores the used Bazel version as metadata.
aggregate_incompatible_flags_test_result.py reads this value and files a GitHub issue for every project and every flag that needs migration.

Fixes bazelbuild#869
fweikert added a commit to fweikert/continuous-integration that referenced this issue Dec 14, 2019
bazelci.py now stores the used Bazel version as metadata.
aggregate_incompatible_flags_test_result.py reads this value and files a GitHub issue for every project and every flag that needs migration.

Fixes bazelbuild#869
fweikert added a commit to fweikert/continuous-integration that referenced this issue Dec 14, 2019
bazelci.py now stores the used Bazel version as metadata.
aggregate_incompatible_flags_test_result.py reads this value and files a GitHub issue for every project and every flag that needs migration.

Fixes bazelbuild#869
fweikert added a commit to fweikert/continuous-integration that referenced this issue Dec 14, 2019
bazelci.py now stores the used Bazel version as metadata.
aggregate_incompatible_flags_test_result.py reads this value and files a GitHub issue for every project and every flag that needs migration.

Fixes bazelbuild#869
fweikert added a commit that referenced this issue Dec 18, 2019
bazelci.py now stores the used Bazel version as metadata.
aggregate_incompatible_flags_test_result.py reads this value and files a GitHub issue for every project and every flag that needs migration.

Fixes #869
@fweikert
Copy link
Member Author

Tomorrow's pipeline run will use this feature.

@fweikert
Copy link
Member Author

fweikert commented Dec 19, 2019

Several issues were created, but there are still some action items:

  • Display correct Bazel version
  • Fix markdown links
  • Improve issue text (some users were confused)
  • Investigate why projects were notified about a seemingly unrelated breakage
  • Add "no notification" field to downstream dict

@fweikert fweikert reopened this Dec 19, 2019
@philwo
Copy link
Member

philwo commented Dec 19, 2019

I would also provide an easy way to opt-out (per repo), in case some users do not want these issues to be filed automatically. Maybe by asking the user to send us a PR that adds their repo to a blacklist. Even if no one ends up opting out, it’s polite to offer the possibility.

@fweikert
Copy link
Member Author

Yes, I already had a plan for that. I updated the task list accordingly.

fweikert added a commit to fweikert/continuous-integration that referenced this issue Dec 19, 2019
Some users were confused since they haven't been using incompatible flags.

Part of bazelbuild#869
fweikert added a commit to fweikert/continuous-integration that referenced this issue Dec 19, 2019
fweikert added a commit that referenced this issue Dec 19, 2019
Some users were confused since they haven't been using incompatible flags.

Part of #869
fweikert added a commit to fweikert/continuous-integration that referenced this issue Dec 23, 2019
fweikert added a commit to fweikert/continuous-integration that referenced this issue Dec 23, 2019
fweikert added a commit that referenced this issue Dec 23, 2019
@fweikert
Copy link
Member Author

Example of an incompatible flag without target release: https://github.com/fweikert/bugs/issues/313
Example of a flag that will be flipped in 3.0: https://github.com/fweikert/bugs/issues/329

fweikert added a commit to fweikert/continuous-integration that referenced this issue Dec 23, 2019
fweikert added a commit that referenced this issue Dec 23, 2019
fweikert added a commit to fweikert/continuous-integration that referenced this issue Dec 23, 2019
fweikert added a commit that referenced this issue Dec 23, 2019
fweikert added a commit to fweikert/continuous-integration that referenced this issue Dec 23, 2019
fweikert added a commit to fweikert/continuous-integration that referenced this issue Dec 23, 2019
fweikert added a commit to fweikert/continuous-integration that referenced this issue Dec 24, 2019
The previous code only fetched the first page of API results, thus missing most of the existing issues.
As a result, it filed duplicate issues on every invocation.

Part of bazelbuild#869
fweikert added a commit to fweikert/continuous-integration that referenced this issue Dec 24, 2019
The previous code only fetched the first page of API results, thus missing most of the existing issues.
As a result, it filed duplicate issues on every invocation.

Part of bazelbuild#869
fweikert added a commit to fweikert/continuous-integration that referenced this issue Dec 24, 2019
The previous code only fetched the first page of API results, thus missing most of the existing issues.
As a result, it filed duplicate issues on every invocation.

Part of bazelbuild#869
fweikert added a commit that referenced this issue Dec 24, 2019
The previous code only fetched the first page of API results, thus missing most of the existing issues.
As a result, it filed duplicate issues on every invocation.

Part of #869
fweikert added a commit that referenced this issue Dec 26, 2019
The script has been tested successfully, which means we can change it to file bugs in the correct repositories (instead of a dummy repo).

Fixes #869
fweikert added a commit that referenced this issue Dec 26, 2019
The script has been tested successfully, which means we can change it to file bugs in the correct repositories (instead of a dummy repo).

Fixes #869
fweikert added a commit that referenced this issue Dec 26, 2019
fweikert added a commit that referenced this issue Dec 26, 2019
@fweikert
Copy link
Member Author

fweikert commented Dec 30, 2019

I wrote a mini post-mortem about the launch of this feature: #915 (comment)

philwo pushed a commit that referenced this issue Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants