Skip to content

Consider CI that prevent simultaneous changes to go_router and go_router_builder #108336

Open

Description

@stuartmorgan-g

Capturing from Discord discussion in the wake of #108274 and flutter/packages#2381 (see also #89518 for background, which was the solution to the same basic problem but with federated plugins in flutter/plugins).

The immediate cause of #108274 was that the CI didn't catch it because of a tooling bug (see #108274 (comment), and the fix for that in flutter-team-archive/plugins#6146). However, based on past experience and Discord discussion, there's a non-trivial chance that if that bug hadn't existed, the flow would instead have been:

  1. Presubmit fails on the initial version of the PR.
  2. The PR is "fixed" to also update go_router_builder to account for the changes.
  3. Both go_router and go_router_builder have new versions published.

This would have almost certainly broken fewer people (including not breaking flutter/packages PRs), but is actually still wrong. Step 2 is incorrect because it violates semver: go_router 4.2.1 would still have been published with changes that would break existing versions of go_router_builder, and there's nothing that requires that someone who updates go_router also update go_router_builder if the constraints don't say it's required (which they would not have here). And it would also be broken the other way: someone updating go_router_builder but not go_router would have been broken, because go_router_builder couldn't have expressed that it required >= 4.2.1 because 4.2.1 wouldn't be published to depend on.

The safest way to avoid that would be to prevent changes to go_router_builder in the same PR that changes anything in go_router's lib/, as we've done with federated plugins. I'm not sure how though for this pair how often this would result in false-positives that would be problematic (e.g., you want to update code comments in one package while making actual changes to the other).

/cc @johnpryan @chunhtai @loic-sharma

Activity

  1. added
    p: toolingAffects the flutter_plugin_tools package
    packageflutter/packages repository. See also p: labels.
    on Jul 26, 2022
  2. johnpryan commented on Jul 26, 2022

    @johnpryan
    Contributor

    It might be possible to fix go_router_builder in a backwards compatible way, see https://github.com/flutter/packages/pull/2381/files#r930121119

  3. chunhtai commented on Jul 26, 2022

    @chunhtai
    Contributor

    we could have a github bot that only flags the pr but don't block it from merging. I saw we do update some meta data for all packages in the flutter/package from time to time. those will be an annoying false positive

  4. stuartmorgan-g commented on Jul 26, 2022

    @stuartmorgan-g
    ContributorAuthor

    It might be possible to fix go_router_builder in a backwards compatible way

    This suggestion isn't about the specific PR, it's about the long-term maintenance of the packages. These packages haven't existed for all that long and this has already been a problem at least twice. It's likely to happen again.

  5. stuartmorgan-g commented on Jul 26, 2022

    @stuartmorgan-g
    ContributorAuthor

    I saw we do update some meta data for all packages in the flutter/package from time to time. those will be an annoying false positive

    The federated plugin tooling has a heuristic for that based on the set of packages being touched. In this case, what we'd want is to block/flag/whatever the case where exactly these two packages are being touched. It's very unlikely that someone would make dangerous intertwined PR for these two packages that also included changes to some other random package(s), so it's a low-risk filter.

  6. added
    P2Important issues not at the top of the work list
    on Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    P2Important issues not at the top of the work listp: toolingAffects the flutter_plugin_tools packagepackageflutter/packages repository. See also p: labels.team-ecosystemOwned by Ecosystem teamtriaged-ecosystemTriaged by Ecosystem team

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions