Add Maven registry support

[markelliot]

Fixes #284

Add a Maven registry backend for buffrs:

• Extract a Registry trait
• Encode registry type as a prefix in RegistryUri
• Dispatch to the correct Registry implementation from RegistryUri

[mara-schulke]

Overall im very happy with the code quality and the implementation approach of this pr! Thank you! 😊

My two dots:

1. This PR is missing tests, before merging i'd like to have end to end tests that ensure that: mixed maven and artifactory setups, maven publishing and maven installation are working as expected. You might need to change the e2e test setup slightly to account for having two registries.
2. Left some comments on type organisation and the possibility to use a builder pattern. Would prefer if they get addressed but it's not a blocker to merging if you feel strongly about the current implementation.

[markusz]

Hi Mark,

thanks a lot for your MR. Having Maven support would be awesome.

I can just echo what Mara said: Overall, this is a high quality MR already. I left a few comments (mostly nits).

The main thing would be to make tests work again and add new unit and e2e tests. The test harness of buffrs can be a bit hard to grok at first. I recently wrestled with it myself, so feel free to reach out if you need help.

I also want to point your attention to v0.12.0 which is soon going to be released (see MR). This change will cause merge conflicts with your current branch. I took the liberty to merge your branch back into a dedicated branch of v0.12.0 feature branch and resolve the conflicts to the best of my knowledge. This will hopefully make it easier to merge your changes back to main once finished.

Thanks again for your contribution

[markelliot]

@mara-schulke @markusz ready for another look when you have a minute

[mara-schulke]

There seems to be some CI issues and i have left a last comment, but this is wonderful, thank you mark!

[mara-schulke]

@markusz is this good from your pov?:)

[markusz]

LGTM!

This is a fantastic addition. I really appreciate the clean implementation of the Maven support.I do want to highlight one important detail regarding compatibility that we should be loud about in the release notes:

If I interpret src/registry/mod.rs:L102-111 correctly, reading is backwards compatible, but writing is not. As soon as we write the Proto.lock or Proto.toml with this new version, it will prefix the registry URL with <reg_type>+.

Consider the following scenario:

1. Alice upgrades buffrs to 0.13.0 and makes a change (unrelated to registries, e.g., buffrs add some_new_package).
2. She pushes the changes and the updated lockfile.
3. Bob (and potentially some automated tools like CI runner, etc.) still on 0.12.0 will be unable to use the new Proto.lock because the older client won't parse the prefix.

I think this sort of breaking change can't really be avoided in this case, but we should be very explicit in the docs that this update requires a lockstep upgrade for teams sharing a repo.

@mara-schulke

[markelliot]

If you want we can probably make the writer always emit no regtype for artifactory?

[markusz]

Valid point, but I feel we should value internal consistency over backwards compatibility in this case. Introducing a one-time breaking change as discussed above imo is more digestible than having this long term inconsistency in the Proto files.

Another option would be to make this somehow optional (CLI flag, ENV variable, ..) but that opens another can of worms.

I'm inclined to merge and be very vocal about the change

@markelliot @mara-schulke WDYT?

[markusz]

Ah, I forgot: Cargo.toml needs to be adapted to reflect the new version. Should we go for 0.13.0? I'm a bit undecided on this. In the narrow SemVer sense this contains a breaking change and should become 1.0.0, but that feels wrong. It's certainly not a patch release either, so I guess 0.13.0 is the best option?

[mara-schulke]

Thank you, this is a good catch @markusz - I'd argue we open a tracking issue for 1.0 in GitHub and note this as a breaking change to make in there. From my pov this is would be a breaking change without a strong requirement to be one which seems to push friction downstream for no apparent reason beyond minimally cleaner code.

buffrs is for the most part stable and only missing an advanced dependency resolution mechanism (think pubgrub) for us to make it to 1.0, id like to batch this change with other breaking changes towards the 1.0 release:)

[markelliot]

Let me know if there's anything else on this one

[markelliot]

@mara-schulke @markusz do you want to proceed here?

[mara-schulke]

Hi @markelliot, yes of course! Sorry I have missed this. Can you confirm that the default behavior is this to omit the artifactory prefix when writing? I'm happy with merging this:)

[markelliot]

That’s not the default behavior, it can be. If I do that cleanup and resolve the new merge conflicts will you be ready to merge then?

[mara-schulke]

yes!

[mara-schulke]

Hi @markelliot can i help you with getting this over the line?:)

[mara-schulke]

@markelliot i will revive this PR for you and get it merged for you

[markelliot]

@mara-schulke I've rebased and cleaned up the conflicts

[markelliot]

also for full disclosure we ended up going down a different path than using buffrs, so also can understand if you'd prefer to close this without merging. I spent the time to clean it up because you had already spent time engaging on reviewing it and it might be a nice buffrs addition anyway.