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

rfc: RFC process #96

Merged
merged 3 commits into from
Aug 19, 2024
Merged

rfc: RFC process #96

merged 3 commits into from
Aug 19, 2024

Conversation

JP-Ellis
Copy link
Contributor

@JP-Ellis JP-Ellis commented Jul 4, 2024

This PR proposes a request for comment (RFC) process to propose and discuss changes to the Pact specification, ecosystem and community.

The PR can be read in the rfc/rfc-process branch

Signed-off-by: JP-Ellis <[email protected]>
@JP-Ellis JP-Ellis requested review from mefellows and YOU54F July 4, 2024 01:28
@JP-Ellis JP-Ellis self-assigned this Jul 4, 2024
rfc/0000-rfc-process.md Outdated Show resolved Hide resolved
rfc/0000-rfc-process.md Outdated Show resolved Hide resolved
rfc/0000-rfc-process.md Outdated Show resolved Hide resolved
@YOU54F
Copy link
Member

YOU54F commented Jul 4, 2024

Thanks Josh, this feels like a great start!

@JP-Ellis
Copy link
Contributor Author

JP-Ellis commented Jul 5, 2024

Thanks @YOU54F! I've update the text based on your comments 👍

rfc/0000-rfc-process.md Outdated Show resolved Hide resolved
@YOU54F
Copy link
Member

YOU54F commented Jul 5, 2024

Thanks @YOU54F! I've update the text based on your comments 👍

danke muchly

Primarily around the list, which was perhaps a little confusing with the
inclusion of 'various languages' but was immediately followed by a
disclaimer that the language implementations are out of scope.

Signed-off-by: JP-Ellis <[email protected]>
4. Submit a pull request. The pull request is where the RFC will be discussed, and reviewed.
5. Incorporate feedback, and iterate on the RFC.

Eventually, a member of the Pact Foundation will either reject the RFC by closing the PR, or accept it by merging the PR. If the RFC is accepted, the following steps will also be taken:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we define who are the members that have decision/voting rights, or is that going over board?

Copy link
Member

@mefellows mefellows left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great start! I don't think we need to overthink it. We can always change it through PRs or further RFCs.

Copy link
Member

@mefellows mefellows left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great start! I don't think we need to overthink it. We can always change it through PRs or further RFCs.

@mefellows
Copy link
Member

I've also tagged several other maintainers for their visibility.

@adamrodger
Copy link

Firstly, I think this is a step in the right direction so thanks for that 👍 Whilst a lot of the development happens in the open, I feel like a lot of the decision making happens in private, so anything we can do to make that more public is a good move.

With that in mind, my first question is - will everyone be taking part in this RFC process? As in will Pact Foundation and SmartBesr employees also be making changes solely via this mechanism, or is this an avenue only for those external?

My second question surrounds the areas covered vs not. It makes total sense to me that the spec is covered by this process, but other parts are less clear. For example, why is the pact CLI covered by this but something like pact-jvm isn't? To an outsider, that's an unclear distinction.

Third point - what happens when an RFC is accepted? For example, a spec change RFC is accepted - is the spec now changed? Do we publish a new version bump? Or is this just accepting the idea of making that change at some future point?

Similar with the CLI and other elements mentioned in the text - if someone says "the CLI should start doing X" and that gets accepted, what then? Obviously someone needs to actually make a code change, and if nobody does then what does accepting the RFC actually mean?

In PactNet the RFC is like the initial design discussion, and is expected to be accompanied with a PR once the design has been agreed, and only then closed. So it's an atomic single ticket that results in a code change, whereas this didn't read like it was. This felt like it was accepting a plan to make a code change somewhere else later.

@JP-Ellis
Copy link
Contributor Author

@adamrodger Thanks for your feedback and for recognising this as a step in the right direction 👍. We definitely want to avoid the appearance of decision-making happening in private, and we hope this RFC process will help with that.

To answer your first question: It is my hope that all significant changes go through the public RFC process, irrespective of who is suggesting the change. This includes contributions from open source contributors and SmartBear employees alike.

Regarding your second question: I agree that this distinction can be difficult to precisely define. The reason is that we want to acknowledge that Pact is an ecosystem with many moving parts, and it isn't reasonable or practical to consolidate all changes through a single RFC pipeline. For instance, you mentioned how Pact Net has its own RFC process; I believe changes solely contained within Pact Net should continue using its own process. Similarly, if someone suggests a breaking change to Pact Python that doesn't affect other implementations, this RFC process might not be the right forum for that discussion.

Conversely, a change to the FFI or the CLI could have much broader ramifications across the ecosystem thereby justifying the need for these changes to go through the RFC process. Though even there, only changes which have these broader ramifications would require this RFC process. A refactoring of logic internal to the FFI for example would not fall within the purview of this RFC process.

I'm open to hearing your thoughts on this or any suggestions you might have on making this distinction clearer.

As for your third point: The idea, which I thought I explained in the RFC process but may need further fleshing out, is:

If the RFC is accepted, the following steps will also be taken:

  1. Assign an RFC number based on the PR number and update the filename and metadata.
  2. Create a tracking issue in the Pact Foundation Roadmap repository for implementing the RFC. Where applicable, other issues may be created in other repositories.

Step (1) is purely administrative. Step (2) aims to track implementation. Given the breadth of possible changes, it's not feasible for one person to implement everything across different parts of the ecosystem. It is also possible for an RFC to come from an end-user of Pact who might have a valid suggestion, but not the technical experience to implement the change.

It's always a concern that an approved change remains in limbo; however, whether it stays as an open RFC or as a tracking issue doesn’t make much difference—except that a tracking issue formally recognises that an RFC has been approved.

Looking forward to hearing more of your thoughts!

@adamrodger
Copy link

On the topic of which components are included and which aren't:

I think your explanation above is a good one, like this process covers the common elements of the ecosystem (such as the Spec and FFI) and not the language-specific parts. That does put the CLI in a bit of an awkward inbetween though. That doesn't feel like it fits the definition of a common element that affects the entire ecosystem.

I think in reality the list of things covered in this process is actually "the things maintained by SmartBear people" vs. "the the things that aren't" generally speaking (I accept it's not that clearly separated, e.g. PactJS maintained by Matt). Where those language-specific libraries are maintained by SmartBear employees then they do tend to pop up in this repo, such as pact-foundation/pact-js-core#521, which is PactJS-specific.

As a non-SmartBear member of Pact Foundation, my two concerns with that are:

  • Marking your own homework - There are enough 'core team' Pact Foundation members that it's very easy for one to raise an RFC and multiple others to comment/approve so that the entire process is followed, yet without involving anyone from outside of that group. For someone outside the group, that's not possible. To the community at large it's unclear who is paid to maintain Pact as their job and who is a volunteer, so it's not really possible to spot when this might happen.
  • Tail wagging the dog - There have been times where the reference implementation doesn't follow what the spec says, and then instead of changing the implementation the spec is updated to match what the implementation does (without any version bump or anything either). This effectively means that the spec can be de-facto updated by proxy. This is especially concerning for library maintainers because then users point to the spec and say you're not compliant, but once upon a time you were.

On the topic of what happens after an RFC is accepted:

I get the difficulty here. We want the RFC list to act as a decision log, and for that the designed process looks fine to me. However, accepting an RFC doesn't actually make that change happen, and it's unclear from reviewing the decision log which ones have and haven't been implemented. Perhaps some kind of extra tracking information needs to be added and maintained there? Like if we're going to raise tickets in other repos to actually implement a decision, the original 'umbrella' RFC should track its own progress against those tickets.

One problem with that approach is that you can get something partially implemented. For example, if we went back in time to before plugins existed then you'd expect that to have gone through an RFC process because it's a wide-reaching change. However, plugin support is still patchy across the ecosystem (generally it's implemented in the core team maintained libs, and not in the volunteer-maintained ones). So what does that mean for the RFC? Is it done? Partially done?

I think if I were an external user that went to the effort of raising an RFC and then it was accepted, that kinda feels like a promise being made to implement that change. If we get to a point where we have it partially implemented, or even not at all, then I don't think that user would engage with the RFC process again. It'd be quite a lot of effort to go through if it then ends up being accepted but not actually implemented.

Perhaps that implies some kind of "agreed in principle" type state of an RFC instead of just "draft" and "accepted"? As in we agree that the idea is a good one and we've ironed out any flaws, and now it moves to a stage where we actually need it implementing. Only after the implementation is the state something like "adopted".

@JP-Ellis
Copy link
Contributor Author

JP-Ellis commented Jul 17, 2024

Thanks for expanding on the feedback 👍

Regarding the distinction of what is and is not under the purview of this RFC process, I think we are in general agreement. The CLI is perhaps not the best example, though I did include it because the current (soon to be deprecated) implementation of Pact Python for example uses the CLI quite heavily under the hood1.

I also hope that this RFC process forms a good foundation; however, I do not expect it to be immutable. If there is a common misunderstanding or failure of the process that becomes evident, an RFC to update the RFC process would be welcome.

On the topic of the implementation, my explanation of the process does mention in passing that a tracking issue is to be created in this repo, though I will admit that it may not be very clear. My plan to address this will be to:

  1. Require that a link to the tracking issue be added to the metadata section of the approved RFC. It will be the responsibility of the person approving the PR to create the tracking issue and update the approved RFC document with the relevant link.

  2. Add a GitHub issue template for the tracking issue. In contrast to the RFC process which details the 'what' and 'why' in detail, I would anticipate the tracking issue to focus more on the 'how'.

    The issue's comments would be a good place for discussion about the implementation itself, in case there are any unforeseen difficulties or recommendations.

    An example of a tracking issue that I am currently using is Pact FFI Tracking Issue pact-python#396 which tracks Pact Python's progress to being fully backed by the FFI library. You can quite easily see the progress (as an "X out of Y tasks complete") in various places of GitHub interface.

I personally would avoid having an "agreed in principle" state for an RFC, as it is ambiguous to me (and may be even more ambiguous to non-native speakers of English). More specifically, I think that an "agreed in principle" does not really explain whether it is to be implemented now, later, or not at all. E.g., it could be agreed in principle, but some time in the future; or agreed in principle, but too difficult to implement; etc. I think an RFC should have three distinct states:

  • draft (PR is open)
  • accepted (PR is merged), in which case a tracking issue is created thereby creating two sub-states:
    • not yet fully implemented (tracking issue open)
    • fully implemented (tracking issue closed)
  • rejected (PR is closed).

Once accepted, the implementation is recorded in the tracking issue I mention above.

Furthermore, if a RFC can only be "accepted" once it is fully implemented, how should we define "fully implemented"? If it requires changes in all Pact implementations across all languages; do we really want to wait until the lesser-known/lesser-used implementations also adopt it before it is marked as accepted? Or do we only care about the "main" implementations (and then the question is, which ones are they)?

I hope I have explained my reluctance to having any additional states to an RFC beyond what is strictly necessary; and I hope that the tracking issue provides an acceptable alternative which will fill the niche.

I will update the PR with my suggestions changes tomorrow morning (as it is late here in Melbourne), but do you think these would address your concerns?

Footnotes

  1. The newer Pact Python V3 rewrite makes use of the FFI library, as having what effectively is a wrapper to a CLI is really not ideal.

@JP-Ellis
Copy link
Contributor Author

@adamrodger I have updated the PR to address your concerns. I hope this helps address the concerns you raised, and if not, let me know.

@mefellows
Copy link
Member

I think your explanation above is a good one, like this process covers the common elements of the ecosystem (such as the Spec and FFI) and not the language-specific parts. That does put the CLI in a bit of an awkward inbetween though. That doesn't feel like it fits the definition of a common element that affects the entire ecosystem.

Yeah, I see this point. The CLI (like the Pact Broker) is something we recommend all users use, so in part, it feels like it should be here and any substantial change to it communicated and agreed upon. For example, if the author of the CLI decided to change the way arguments were handled (because Thor is a bit...strange) should that be raised here or not? I think so?

The additional of a new subcommand that fits with the existing model (e.g. to list the currently deployed applications for an environment)? Maybe.

The broker seems more important. For example, when the "pacts for verification" feature was created, there was a tracking issue created in the pact_broker repo - but raising it here would have made it more visible to maintainers. It's an important new concept introduced into the ecosystem, that will ripple into other languages and sits outside of the spec. It will eventually result in new FFI APIs etc.

I think in reality the list of things covered in this process is actually "the things maintained by SmartBear people" vs. "the the things that aren't" generally speaking (I accept it's not that clearly separated, e.g. PactJS maintained by Matt). Where those language-specific libraries are maintained by SmartBear employees then they do tend to pop up in this repo, such as pact-foundation/pact-js-core#521, which is PactJS-specific.

That might be true, but only coincidentally so. If the rust core or the CLI were to have additional maintainers external to the SmartBear team I wouldn't expect the process to change. The aim is to have a place where core features that affect the whole ecosystem should be raised, and not for any language/repo/SDK-specific request.

That issue might be a red herring . All of the feature requests previously in Canny were moved here to move away from Canny and to something first-class in the ecosystem (i.e. GitHub). Pact JS-specific feature requests should go to https://github.com/pact-foundation/pact-js like any other feature request. That being said, that is an example of a feature request that may be raised as an RFC for support in other languages, as it is a feature in Pact JVM that a user would like to have supported in Pact JS (metadata support.

As a non-SmartBear member of Pact Foundation, my two concerns with that are:

Marking your own homework - There are enough 'core team' Pact Foundation members that it's very easy for one to raise an RFC and multiple others to comment/approve so that the entire process is followed, yet without involving anyone from outside of that group. For someone outside the group, that's not possible. To the community at large it's unclear who is paid to maintain Pact as their job and who is a volunteer, so it's not really possible to spot when this might happen.

I understand this point. I'm also concerned with the reverse problem. If we don't have clarity on the process to approve/reject an RFC, it might sit around for ages with no progress. For example, with the Plugins RFC we had basically zero contributions from the maintainer group at large. Back then, we didn't have a process like we are proposing now, but it was added to the V4 spec RFC where it was visible and maintainers did comment, it was made visible in our maintainers channel etc. For 6 months (maybe more) we had no feedback before it was started and then only after implementation did we get critical feedback (which was obviously too late).

Without re-litigating that history, we want to avoid an RFC sitting around that could be progressed but isn't because maintainer X or Y hasn't had time to get around to it.

Tail wagging the dog - There have been times where the reference implementation doesn't follow what the spec says, and then instead of changing the implementation the spec is updated to match what the implementation does (without any version bump or anything either). This effectively means that the spec can be de-facto updated by proxy. This is especially concerning for library maintainers because then users point to the spec and say you're not compliant, but once upon a time you were.

I would like to think this RFC process would start to address that kind of problem.

@JP-Ellis
Copy link
Contributor Author

Important

Deadline for comments:
Sunday 18 August

Thanks everyone for the feedback on the RFC thus far!

With the PR being already 3 weeks old, I am suggesting leaving open for another 3 weeks to give everyone the opportunity to provide feedback. I am suggesting a cut-off date of Sunday 18 August.

After that date, and provided that any new comments are appropriately addressed, I intend to merge this PR so that we have a formal RFC process!

@JP-Ellis
Copy link
Contributor Author

Final reminder that the deadline for any further comments is in 3 days!

@JP-Ellis JP-Ellis merged commit 90cd688 into master Aug 19, 2024
1 check passed
@JP-Ellis JP-Ellis deleted the rfc/rfc-process branch August 19, 2024 22:35
@mefellows mefellows mentioned this pull request Oct 17, 2024
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

Successfully merging this pull request may close these issues.

5 participants