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

Publish pre-releases from master builds / Trigger dependant client libraries workflows to raise PR's #375

Open
YOU54F opened this issue Jan 24, 2024 · 8 comments

Comments

@YOU54F
Copy link
Member

YOU54F commented Jan 24, 2024

The current ecosystem is large and we are currently growing the number of client libraries consuming the pact ffi library.

In some cases, the test suites of the consuming the pact ffi library can throw up errors, not evident in this repo's test suite.

I propose a couple of measures that could improve the feedback loop.

  1. On release of Pact FFI / Verifier CLI / Mock Server CLI, raise a PR against the consuming libraries repositories, updating to consume the new library
    1. Ecosystem Map
    2. Process in Ruby
      1. Trigger
      2. PR Raising Workflow & Script
  2. In order to get quicker feedback loops, prior to an official release, we could pre-release against pushes to master, which could trigger the respective client libraries workflows. Maybe this would be better named next or something

Thoughts on a postcard. I would really like to do point 1, that brings us to line with the old pact ruby core.

Point 2 should be considered, maybe not the implementation but that forward feedback, I feel would be valuable for myself, and if anything was uncovered in the respective client libraries that could be found earlier in this repo, we could add additional test cases.

@adamrodger
Copy link
Contributor

I think if we're going to do this then the release notes and comms to client library maintainers need to be improved as a prerequisite. Probably also the FFI needs to hit 1.x and follow proper SemVer (automatic raising PRs against a bugfix release is much easier than raising against new features or breaking changes).

It's very hard to know release-to-release exactly what's changed, and given FFI is still at 0.x it just changes really rapidly. Functions get deprecated or 'v2' versions added, even in some cases the entire interaction model has changed between versions. I wouldn't want to inherit all of this churn frequently.

The release notes often say "upgraded pact-models to 0.1.2" or whatever, which means you have to follow the change logs into all the other libraries as well to work out what's included in a new FFI version.

Even then sometimes you need to actually go and read all the code because the commit message doesn't tell you what changed or why - I've literally just finished checking the code for a commit with the message "stupid Windows", which isn't helpful to me at all.

I'm sure that PactNet isn't using the absolute latest of everything because the maintenance burden is just too high when you've got to follow all that discovery and evaluate the effect of the churn every time. If I had monthly PRs being raised automatically then that forces me to go through that process of working out for myself what's changed far too regularly.

That's of course on top of all the other things you need to do as a client library maintainer - a new version of .Net comes out, a dependency has breaking changes etc and you need to keep up with those changes, then rapid FFI change at the same time is a big burden.

If it was easy then that's less of a burden, but currently it's too hard unless it's literally your day job so you are always in the loop. I think better release notes (which can be attached to a PR) are how that gets eased, and I think that probably needs the library to be at 1.x so that we can more easily create those targeted release notes.

@mefellows
Copy link
Member

mefellows commented Feb 15, 2024

I suppose that just raises another question we should consider - what do we need to do to get it to a 1.x.x?

Perhaps that can be the topic of an upcoming maintainer meeting?

(proposed it here)

@YOU54F
Copy link
Member Author

YOU54F commented Feb 15, 2024

Thanks Adam, that is really useful feedback as always.

It's very hard to know release-to-release exactly what's changed, and given FFI is still at 0.x it just changes really rapidly. Functions get deprecated or 'v2' versions added, even in some cases the entire interaction model has changed between versions. I wouldn't want to inherit all of this churn frequently.

The release notes often say "upgraded pact-models to 0.1.2" or whatever, which means you have to follow the change logs into all the other libraries as well to work out what's included in a new FFI version.

Even then sometimes you need to actually go and read all the code because the commit message doesn't tell you what changed or why - I've literally just finished checking the code for a commit with the message "stupid Windows", which isn't helpful to me at all.

Yep, I can empathise with this. I do think there is some improvements that we make so provide a summary of the changes, outside of the commit logs. I do find it a bit of a chore to traverse the release notes, with commit refs and tracing them down to individual crates, and working out the changes from commit messages, without reading the code.

I wouldn't want to inherit all of this churn frequently

Apologies, my intention wasn't really to force the maintainers of the client libraries to inherit this churn, but to provide a quicker feedback loop between releases of the pact-ref project, and the impact on client libraries (and conversely maintainers)

Would a release tracking issue, that has a summary of the release contents, impacts/changes for maintainers be useful, alongside a place to discuss any issues, and provide links to PR's in consuming repos. I'm wondering if seeing a PR of the new release consumed in another client library, help you in some way as a maintainer?

@adamrodger
Copy link
Contributor

It's a weird chicken-and-egg thing, because effectively you want to test pre-release versions of the FFI against all the client libraries to spot any issues early, but the client libraries don't want to accept pre-release versions of the FFI, only 'full' releases. But waiting until they're properly released is just asking for finding breaking changes too late 😁

A PR turning up really depends on what the contents are. If it's a bump from 1.2.3 to 1.2.4 then no big deal. We're only expecting bug fixes, there shouldn't be any functional change at all, and certainly no breaking changes. Those should pretty much fly through.

Any minor or major change would be more invoved though, and needs to come with some kind of summary of what's changed and what impact that may have, so that's when more detailed release notes are needed.

I've been going through the FFI this afternoon updating PactNet from 0.4.5 to 0.4.16, for example, and lots of things have changed. I've replaced something that was now deprecated, but other bits I'm confused as to how they're even supposed to be used, and the docs/examples often still refer to the deprecated bits so I've basically just ignored the new confusing bits for now.

That's a related problem to this one though, because the 'best practice' usage guide of the FFI needs to be really obvious, and changes to that called out in a clear way each time they happen. Once we get to that point then raising PRs automatically with those changes included becomes a possibility.

As for the actual question of what those PRs contain, that's a tricky one. When a method is deprecated or just no longer recommended (I'm thinking the v2 versions of methods, or the multiple different interaction models that now exist), would we expect the PR to contain the C# (for example) changes required to support them? If not, and the C# continues using the old deprecated versions of things, I'm unclear what the value in raising the PRs is, because the new stuff isn't being tested anyway.

So, automated PRs for bugfix releases (once we're at 1.x and beyond) makes sense to me. Automated PRs for major/minor releases I think is much more tricky, and in general I think that entire process probably needs some more thought for maintainers more like myself who don't work for Pactflow/Smartbear/etc and are instead volunteers. The FFI churn is currently high and it's very easy to lose track of what's changing and what you need to do in response currently.

@mefellows
Copy link
Member

As for the actual question of what those PRs contain, that's a tricky one. When a method is deprecated or just no longer recommended (I'm thinking the v2 versions of methods, or the multiple different interaction models that now exist), would we expect the PR to contain the C# (for example) changes required to support them? If not, and the C# continues using the old deprecated versions of things, I'm unclear what the value in raising the PRs is, because the new stuff isn't being tested anyway.

Deprecations and new features/methods will always happen with libraries, I don't think that should be the reason we wouldn't auto-open a PR even in such cases where the only changes will be these sorts of things (i.e. no additional value for the client project). In most cases, the release will include changes that fixes bugs or adds improvements to the existing features.

I think the key point would be ensuring the key new features / changes are linked along with the PR. That way, the commit could contain breadcrumbs to all of the changes (instead of the more opaque "fix: update FFI core to version x.y.z" type thing).

@adamrodger
Copy link
Contributor

adamrodger commented Feb 18, 2024

Yeah that's why it needs to be at 1.x first though. You don't need to bump the major version just to deprecate something, but you do need to raise a major version change in order to actually remove them.

If a PR is raised and contains both bugfixes and breaking changes (which 0.x releases always allow) then the PR process in general has much more friction. If I want the bugfixes I have to take the breaking changes at the same time, and they may be non-trivial. That delays getting fixes out for client libraries, for example.

If it was at 1.x then we can produce both a 1.0.1 with just the fixes to allow client libraries to upgrade quickly, and a 2.0.0 to introduce the breaking changes separately in a more considered way. If any new fixes are needed we can produce 1.0.2, 1.0.3 etc. whilst we're waiting for client libraries to migrate to 2.x. That's much less friction than the all-or-nothing 0.x approach.

@mefellows
Copy link
Member

As discussed in the maintainer meet yesterday, here are my thoughts on this issue:

FFI is already a de-facto 1.x.x

Multiple languages now rely on it (JS, Go, .NET and soon Python and PHP), so any breaking changes between patch versions are felt regardless of the actual naming scheme.

Josh raised that there are some inconsistencies in FFI functions

  1. Return arguments may be void, bool or int.
  2. Mixed use of pointer types and handles
  3. Mixed use of logs vs fetching via string helpers

(@JP-Ellis I probably have gotten some of these points wrong, please correct me here / edit this post as needed).

We may want to address these - and any others we believe should be tackled now - in a single hit, and then pull the trigger on a 1.x.x. This will give a stable upgrade path a go.

Opaque Release Notes

As Adam raised above, one challenge of simply bumping the FFI version in a client language is that it's not clear to end users what a given version of the FFI brings to the changelog of that client. It makes it harder for them to see what's new, trace the source of bugs etc.

If the PR could contain the upstream changes (that might be in turn, collected from other pact-reference changes) that would improve this situation.

I agree with this point.

Equally, however, I don't think that should stop us from automating the PRs assuming we had a stable release line and pushed backwards compatible changes. The Ruby updates didn't contain this, and were universally welcomed by maintainers. In some cases, regression suites in the client languages turned up a problem, which meant we could quickly revert or patch breakages. If nothing else, this is a great reason to institute this change.

New feature awareness for the maintainer

The corollary to the above point is to improve visibility for the maintainer. Perhaps sharing a diff of the headers file could be a way to scan for new major features that are now available to the maintainer that could spark them to create a feature request knowing the capability now exists.

@JP-Ellis
Copy link
Contributor

You've summarised my points quite well!

Specifically regarding the inconsistency in the interfaces, while I think it would be nice to have to a 1.0.0 release, this would also be a significant breaking change for the downstream libraries. I think such a change would also require community input to make sure we have a mutually agreeable standard.

So even if we did mark what we have now as 1.0.0, the above can still be achieved at a later time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants