Azure/azure-rest-api-specs

[TypeSpecRequirement] Stable versions in main should only count as brownfield if no stable suppressions

mikeharder opened this issue · 1 comments

mikeharder commented

The check currently uses the following algo:

  1. If your spec is TypeSpec, you are good
  2. Else, you need some reason
    1. A suppression for exactly this version in suppressions.yaml
    2. A "stable" in main, which was intended to mean you shipped a stable version before TypeSpec was required

The problem is step 2.ii. is no longer true, since the version might have been suppressed. Ideally, we would check each version in main, against suppressions.yaml in main, but this would be very expensive. As a proxy, I think we can check if the current branch contains any suppressions for TSR for any stable versions. New algo:

  1. If your spec is TypeSpec, you are good
  2. Else, you need some reason
    1. A suppression for exactly this version in suppressions.yaml
    2. A "stable" in main, that is not suppressed, which means it existed before TypeSpec was required and is a permanent exemption
      1. We don't want to pull main or check all the folders under stable. As a proxy, we will check if there are any suppressions for any stable folders in the current spec.

Put another way (I believe is at least mostly equivalent):

  • A spec is greenfield if it has no stable versions in main, or has at least one stable TSR suppression
  • A spec is brownfield if it has a stable version in main, and no stable TSR suppressions
konrad-jamrozik commented

Some remarks.

Re:

[You are requesting a TSR exemption and your spec has] "stable" in main, that is not suppressed, which means it existed before TypeSpec was required and is a permanent exemption

There is a special case here where:

  1. The PR author is requesting TSR exemption
  2. AND a spec may have stable in main, that is not suppressed, and yet may have been added after TypeSpec was required.

This scenario may happen when someone went through the process of converting their service specs to TypeSpec and for whatever reason is trying to publish new API version that is a manually written OpenAPI spec instead of being emitted from TypeSpec. I think this is unlikely scenario and we don't need to protect against this, but perhaps we could consider adding some telemetry to detect cases like this post-factum.

In other words, we assume/expect that a service is either brownfield and thus TSR skips over it reporting success (and the service has no suppressions - more on this later), or if a service is greenfield, then it either has no TSR suppressions at all, xor its first published stable API version has TSR suppression, and the next consecutive stable API version have suppressions up to some version, and then never again, because at some point the specs have been converted to TypeSpec. I am not exactly sure what we expect here from preview API versions.

Re:

We don't want to pull main or check all the folders under stable. As a proxy, we will check if there are any suppressions for any stable folders in the current spec.

Does it mean we need to find all suppressions.yaml files in the repository? Or do we have (or need) some kind of validation on the paths within the suppression files that forbid the paths from trying to suppress API version from sibling specs?

For example, specification/foo/suppressions.yaml could try to suppress TSR for API version at ../bar/(...). Do we protect against this case?

Re:

A spec is greenfield if it has no stable versions in main, or has at least one stable TSR suppression

You did not include preview TSR suppressions. So this would suggest that a spec that has preview TSR suppression may not be considered greenfield. So it may be brownfield. But why brownfield specs would require preview TSR suppressions? I understand that if a spec is considered brownfield by TSR, there is never any need to suppress any API version of it.

I would say we could strengthen this to:

A service spec is greenfield if it has no stable version in main, or there exists at least one suppression in any of the suppressions.yaml files whose path matches to at least one API version of the service, stable or preview.

It is still possible that someone adds a preview (or even stable) API version suppression to brownfield spec. Maybe to keep the repository clean we should block such PRs with message along the lines "what you did makes no sense, remove this suppression to avoid confusion" ? If not, maybe we should at least have some telemetry to find such cases post-factum.

Re:

A spec is brownfield if it has a stable version in main, and no stable TSR suppressions

Same as above, you did not include preview TSR suppressions. So this would suggest that a brownfield spec may have a preview TSR suppression. But this doesn't make sense - why ever suppress TSR for brownfield spec? TSR will just pass for such specs. Hence we may consider blocking such scenarios as "nonsensical". I elaborated on this above.

I think we could thus strengthen this:

A service spec is brownfield if it has a stable version in main, and there exists no suppression in any of the suppressions.yaml files whose path matches to any of API versions of the service.

With this we need a third category of specs that we may want to prevent from happening:

A service spec is invalid, i.e. not brownfield nor greenfield, if it has a stable version in main, and there exists at least one suppression in any of the suppressions.yaml files whose path matches to at least one API version of the service.

So the spec definitions together:

  • A service spec is greenfield if:
    • it has no stable version in main,
    • OR there exists at least one suppression in any of the suppressions.yaml files whose path matches to at least one API version of the service, stable or preview.
  • A service spec is brownfield if:
    • it has a stable version in main,
    • AND there exists no suppression in any of the suppressions.yaml files whose path matches to any of API versions of the service.
  • A service spec is invalid, i.e. not brownfield nor greenfield, if:
    • it has a stable version in main,
    • AND there exists at least one suppression in any of the suppressions.yaml files whose path matches to at least one API version of the service.