-
Notifications
You must be signed in to change notification settings - Fork 46
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
Comments
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. |
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) |
Thanks Adam, that is really useful feedback as always.
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.
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? |
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 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. |
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). |
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. |
As discussed in the maintainer meet yesterday, here are my thoughts on this issue: FFI is already a de-facto 1.x.xMultiple 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
(@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 NotesAs 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 maintainerThe 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. |
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 So even if we did mark what we have now as |
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.
next
or somethingThoughts 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.
The text was updated successfully, but these errors were encountered: