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

Update on incompatible change process #16195

Closed

Conversation

meteorcloudy
Copy link
Member

Our incompatible change process isn't up to date since Bazel adopts LTS release.

This PR update the process to clarify how an incompatible change should be introduced in Bazel.

Related: bazelbuild/continuous-integration#1410 is the effort to fix the Bazelisk + Incompatible flags pipeline to provide useful information for incompatible flags flip.

@meteorcloudy
Copy link
Member Author

/cc @keith @alexeagle

@meteorcloudy meteorcloudy requested a review from Wyverald August 31, 2022 11:29

## Flipping the flag {:#flip-flag}

Before flipping the default value of the flag to true, please make sure that:

* The migration window is respected.
* Core repositories in the ecosystem are migrated. On the
[`bazelisk-plus-incompatible-flags` pipeline](https://buildkite.com/bazel/bazelisk-plus-incompatible-flags){: .external},
Copy link
Member

Choose a reason for hiding this comment

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

you might need to indent this line and the next

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, done!


* User concerns and questions have been resolved.

When the flag is ready to flip in Bazel, but blocked on internal migration at Google, please consider setting the flag value to false in the internal `blazerc` file to unblock the flag flip. By doing this, we can ensure Bazel users depend on the new behaviour by default as early as possible.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is for Google only. Should that be highlighted in some way?

Copy link
Member Author

Choose a reason for hiding this comment

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

Most incompatible change authors are googler, so I think it's fine.

The incompatible change issue is closed when the incompatible flag is flipped at
HEAD. All incompatible changes that are expected to happen in release X.Y
are marked with a label "breaking-change-X.Y"."
When an incompatible change is ready for migration with Bazel at HEAD (therefore, also with the next Bazel rolling release), it should be marked with the `migration-ready` label. The incompatible change issue is closed when the incompatible flag is flipped at HEAD.
Copy link
Contributor

Choose a reason for hiding this comment

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

Migration ready is a signal that we are committed to the change and it will become permanent, but it is not necessarily in the next LTS release. We need to account for ones like the non-empty glob change, which has been migration ready for years, but we are still not ready to flip.

We should leave the bug open until we delete the code behind the flag, not just flip it. It is not over until it can't be reverted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, migration-ready doesn't imply it will be flipped in the next major release, it totally depends on the migration status and other potential blockers. Having this label tells the incompatible flags pipeline which flags to test so that we can check the migration statuses for downstream projects.

We should leave the bug open until we delete the code behind the flag, not just flip it. It is not over until it can't be reverted.

I agree with it, it's just there are already many issues closed without removing the flag, maybe we should reopen this. I'll update this line.

If a problem is found with the flag and users are not expected to migrate yet:
remove the flags `migration-xx.yy`.
remove the flags `migration-ready`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Making people look at the issue to decide if it is migration ready or not it not going to be a great experience. A retraction of a plan to change behavior has to be acompannied by an announcmented on bazel-discuss that we are changing the plan.

Copy link
Member Author

Choose a reason for hiding this comment

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

Making people look at the issue to decide if it is migration ready or not it not going to be a great experience.

Why? I think it's pretty easy to check what flags are migration ready by just opening this link instead of going through emails.

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 think we can still use the breaking-change-x.0 label to communicate which flags are meant to be flipped for a major release. Added a sentence about that.

Copy link
Member

Choose a reason for hiding this comment

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

breaking-change-x.0 will probably only be useful for archiving purposes (i.e. history), since we only do one major release every 9+ months and it's basically impossible for two open migration-ready issues to be targeted for different major releases.

Copy link
Member Author

@meteorcloudy meteorcloudy Sep 5, 2022

Choose a reason for hiding this comment

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

That is true, but there could be incompatible flags that are not planned to be flipped for the next major release. For example, --incompatible_disallow_empty_glob and --incompatible_strict_action_env, they are ready for migration, but the migration might be very hard or blocked for other reason. By having this label, we can communicate which flags are actually going to be flipped in next major release, so that users can already start turning them on to prevent regression.

/cc @aiuto @alexeagle WDYT? Do you still think a custom bazelrc file is better? Can you parse this label to generate that bazelrc file?

* Core repositories in the ecosystem are migrated.

On the [`bazelisk-plus-incompatible-flags` pipeline](https://buildkite.com/bazel/bazelisk-plus-incompatible-flags){: .external},
the flag should appear under `The following flags didn't break any passing Bazel team owned/co-owned projects`.
Copy link
Contributor

Choose a reason for hiding this comment

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

How will people know they are bazel team owned. The pipeline should be all or nothing. That is, we should have a mandatory list that block a flag flip and an advisory one, which does not block us. It can't be up to analysis.
Unless we change it around and take the choice out of the developer's hands and give the choice to a release manager, who owns an LTS track and can allow or disallow any specific change.

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 information is recored in the ci downstream projects config: https://github.com/bazelbuild/continuous-integration/blob/534ca201c72b713a36afd017ec6bb38aff9cd4b5/buildkite/bazelci.py#L137

The list might be out of date now, but we can easily update them since @sventiffe @jdob and I already reviewed all repos under bazelbuild.

@copybara-service copybara-service bot closed this in 47fa925 Sep 7, 2022
aiuto pushed a commit to aiuto/bazel that referenced this pull request Oct 12, 2022
Our incompatible change process isn't up to date since Bazel adopts LTS release.

This PR update the process to clarify how an incompatible change should be introduced in Bazel.

Related: bazelbuild/continuous-integration#1410 is the effort to fix the Bazelisk + Incompatible flags pipeline to provide useful information for incompatible flags flip.

Closes bazelbuild#16195.

PiperOrigin-RevId: 472731287
Change-Id: I7905d1deb5d9c62f270b888fb9cfe53e36a99179
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Documentation Documentation improvements that cannot be directly linked to other team labels type: process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants