RevApi requires up to date with develop

[felixdesouza]

What happened?

I made some internal code breaks. I resolved the breaks, but this was against an "old" tag i.e. someone released just after I resolved the breaks, so when this merged, I got a semantic merge conflict where I needed to resolve the breaks again and the metadata for the previous tag was ignored.

i.e. develop is compared to 0.162.0 vs 0.161.1 (where I resolved the breaks).

What did you want to happen?

It should recognise the break when it happened vs when it merged into develop, and not require me to be up to date with develop or submit extra prs after develop breaks.

What could we do?

Thoughts are whether we keyed the file by the break instead of the version, and the key can be some sort of hash. This would avoid the case outlined above, as you'd just look up the hash of the break. We could keep the version, but as some metadata, and that would give you the "logical" time when this break was made vs when it merged into develop. But it would be purely for informational purposes, unless there was another use for the version?

e.g. from

acceptedBreaks:
  0.162.2:
    com.palantir.atlasdb:atlasdb-config:
    - code: "java.method.removed"
      old: "method com.palantir.atlasdb.factory.ImmutableLocalPaxosServices.Builder\
        \ com.palantir.atlasdb.factory.ImmutableLocalPaxosServices.Builder::pingableLeader(com.palantir.leader.PingableLeader)"
      new: null
      justification: "this is internal code"

to

acceptedBreaks:
  com.palantir.atlasdb:atlasdb-config:
    0fb0236503:
      - code: "java.method.removed"
        old: "method com.palantir.atlasdb.factory.ImmutableLocalPaxosServices.Builder"
        new: null
        justification: "this is internal code"
        version-introduced: 0.162.2
      # any other hash collisions

@iamdanfox, @CRogers for SA.

[CRogers]

This is a great summary of the problem. The intention of versioning the breaks was twofold:

1. Allow people to easily see which breaks have occurred in a release
   • However, gradle-revapi doesn't currently have an easy way for you to work this out. At best you get this break occurred after version x.y.z
2. Stop people from accidentally re-releasing a break. For example, I delete class foo.Foo in 1.1.0 (and accept the break), reintroduce foo.Foo in 1.2.0 then try and delete it again. Since I've accepted the break of foo.Foo, I don't get warned about "rebreaking" my API.
   • A possible solution is to use the fact people should only make breaks in major versions if they are using semver, but people don't do this and not everyone uses semver, so doesn't work.

I can't think of way in the world of "squash & merge" and not forcing develop to be up to date before merging to get the benefits of 2, so we'll probably have to abandon it as an ideal.

Perhaps you could do something like encode the breaks which actually occurred (and were accepted) into the published jars, but this seems brittle (what if the plugin is disabled for a released or publish fails. Do you have to look back at every jar to work out which breaks are fine?)

It probably makes sense to just remove the versioning entirely as a factor to work out which breaks to ignore. I think this is what the revapi maven plugin and MIMA do too. Maybe keeping the versioning as break-was-applied-after or similar makes sense for informational purposes only.

[felixdesouza]

Hmm, what if you followed a similar strategy to autorelease where we have folder structure like so:

.palantir/breaks
|_ @unreleased
|_ 0.164.0
|_ 0.163.0
|_ <etc...>

At tag time, have a task that will then resolve all the breaks and put them in the correct folder. This would mean that when a tag occurs and things are moved over, squashing and merging will keep breaks in unreleased until it is actually released, giving you exactly when the break occurred, which solves (1) I believe.

I think for (2) if you undo a break and release, that should fail rev-api. In all three instances it is a conscious decision to break/unbreak your api and the hope would be you're not doing it that often, which makes it weird to optimise for.

[CRogers]

Yeah, was just discussing that with @iamdanfox that would be ideal (it also stops merge conflicts), but it does require a lot of work to make it usable. In particular, we'd have to support break-movement in autorelease-bot, despite it not actually being involved in releasing. This also makes it pretty inaccessible for non-Palantir users of this gradle plugin.

I'm not sure how we would support erroring on "unbreaking" your API in gradle-revapi - we'd likely have to "invert" all the previous breaks (java.class.removed -> java.class.added etc) and then make them failures, which would be a bit strange, as we'd now be erroring on none breaking changes.

I agree that people flip-flopping between APIs via breaks in not something to optimize for.

I'm swinging towards just keeping the existing revapi.yml format for storage (to avoid migration) but grabbing all the accepted breaks across all versions.

[felixdesouza]

By support would it not just be invoking the gradle task? In terms of decomp I agree it's kinda weird. I only figured it was okay since we decided that having a separate commit for the release was okay.

Can't the argument for accessibility be made about the changelog? I can't recall if changelog is oss yet or not, so potentially a moot point.

I suppose if we just use revapi.yml with the above form, you wouldn't get merge conflicts anyway (well less likely to) and you could just diff them yourself + expose a gradle task that would tell diffs between versions.

[CRogers]

Autorelease doesn't run gradle code (that's dangerous from a security point of view, the now unused initial version of autorelease we made for hackweek which did this was basically one big security hole). Autorelease has special case code for moving changelogs.

I've been thinking about whether we could include something like a .palantir/breaks/@unreleased/autorelease-instructions.yml that would look something like this:

on-release-move-to: .palantir/breaks/$VERSION

and Autorelease would discover these and move those directories at release time. Would at least not indelibly tie autorelease to revapi specifically.

[felixdesouza]

gotta love the good old RCE, yeah that would be pretty sweet.

[CRogers]

Or, after discussing with @iamdanfox, we could just put it in changelog/@unreleased with the other changelogs, probably formatted as <justification>.revapi.yml

[CRogers]

I had a look around and it looks like it's going to be hard to add support for this in autorelease in a quick + sensible way. Adding breaks to the changelogs folder can be easily supported in autorelease-bot but will cause gradle-changelog to explode. Getting Autorelease to move other folders is going to be a bit more of a challenge and need to get buy-in for.

If we were to migrate to the breaks/ setup, we'd need to do something with the existing accepted breaks in .palantir/revapi.yml. There isn't a good way for us to work out which released versions the accepted breaks should be retroactively moved to, so the file should probably remain. If the breaks/ world ever becomes reality, we could look at the @unreleased breaks and all the breaks in revapi.yml. This also helps non-Palantir users who don't have access to autorelease, as they can just use revapi.yml.

Which gives us a route forward to fix this - just look at all the breaks in revapi.yml, which stops the semantic merge conflict happening and doesn't prevent us from moving to the breaks/ world in the future.

[CRogers]

Or possibly another crazy idea involves reading out the version of revapi.yml from git at the previous tag then selecting only the new breaks added in the current revapi.yml - will have to think about it, but it could give us everything we want!

[felixdesouza]

another crazy proposal, what if it was an extension of the changelog format? it seems pretty reasonable to include api breaks in a changelog!

You'd get PR granularity, no merge conflicts, and autorelease ui + changelog rendering via our excavator could include it!

Dunno about migrations though 😅