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

draft: Remove cli and api #435

Closed

Conversation

TimothyJones
Copy link
Contributor

DO NOT MERGE THIS CASUALLY

This PR was requested by @mefellows as a companion to pact-cli. It is intended to be merged after that is published.

It introduces several breaking changes

This PR removes the binstubs, the Ruby binaries, and the API that exposed them. The following classes are removed:

  • Publisher
  • Message
  • Server
  • AbstractService
  • Stub
  • CanDeploy
  • CannotDeployError
  • (all associated options types).

The following methods have been removed from the default export and new Pact():

  • createServer
  • listServer
  • removeAllServers
  • createStub
  • listStub
  • createMessage
  • publishPacts

The following (all) binstubs have been removed:

  • pact-broker
  • pact-message
  • pact-mock-service
  • pact-provider-verifier
  • pact-stub-service
  • pact
  • pactflow

The documentation for all of the above has been removed. This highlighted that there isn't any documentation for the current consumer flow, and the documentation for the message flow was out of date.

I believe I've removed everything appropriately, but there's always a chance that I missed something or removed something that shouldn't have been removed. It should be reviewed carefully before merging.

  • I have not tested this with PactJS to see if there are changes needed there too. I would expect there are.
  • This PR removes the repository trigger for ruby standalone updates. We should ensure that the repository trigger is connected to the Pact-JS-CLI repo before merging this.
  • Many users are using this API for pact publishing, and it is re-exported by Pact-JS (or, at least, it used to be). There should be a discussion on whether it is acceptable to remove the programmatic use of Pact before merging.

My gut feeling is that users will want to be able to call at least the pact broker programmatically - some users will already have programmatic use baked in to their existing pipelines, as this was the pattern in the examples for a long time. It would be annoying to bring in a breaking change for those users. Removing the programmatic use also means that users can't build cool things on top of pact as easily.

It might be worth considering backing the existing broker API with the Rust implementation instead of removing it

I've included changelog messages, so if this is merged without squashing, the changelog will have the details of what is removed.

…oker, pact-message, pact-mock-service, pact-provider-verifier, pact-stub-service, pact, and pactflow). These have moved to @pact-foundation/pact-cli
… (Publisher; Message; Server; AbstractService; Stub; CanDeploy; CannotDeployError). Also remove the Pact() methods that called them (createServer; listServer; removeAllServers; createStub; listStub; createMessage; publishPacts). If you need these features, please use @pact-foundation/pact-cli
@YOU54F
Copy link
Member

YOU54F commented Mar 20, 2023

Hey @TimothyJones,

Thanks for this, I am doing a call out for it in our newsletter for PactFlow and the open source blog update for Pact, so get some additional eyes and thoughts on this.

@mefellows
Copy link
Member

Thanks, good idea Yousaf.

If we do decide to keep it, I would actually propose we move the programmatic interface to the CLI package also. It can only work with those CLI tools and those CLI tools can only run on certain OS/arch combinations, which is one of the reasons we need to separate it from this package which can run on a wider range of them.

I don't see any harm in keeping it to be honest, with the exception that we aren't putting much engineering work into them and so they are really a snapshot in time for those that want it.

Moving it to the CLI package also means if people want it, they can explicitly opt-in to keeping it.

At the end of this moving about, Pact JS (11.x.x) can have the pact-cli package as an optional dependency and pact-core as a dependency, which means Pact JS can be installed on a wider range of systems, and the optional dependency won't install on those platforms. It's not a breaking change, because it will install on all of the currently supported systems.

@mefellows
Copy link
Member

We haven't got any feedback about this, so I'm in favour of the next major release getting this out the door. It would make sense to do as part of (the release, not the merge) of #446.

@YOU54F
Copy link
Member

YOU54F commented Jul 6, 2023

Yes I think this makes sense now, and as you said we haven't had any feedback either way.

Thanks for kicking this off Tim, will look to extend and release the pact cli asap.

Question - do we want the API to live on in pact-cli package? or is that going to have it's final hurrah in this release of pact-js-core?

@YOU54F
Copy link
Member

YOU54F commented Jul 6, 2023

It might be worth considering backing the existing broker API with the Rust implementation instead of removing it

I have asked about this, but I don't think there is any plan from the maintainers to do this at the moment. I can see some of the rationale, the pact-broker is still written in ruby, but the flip side of we are distributing binaries in rust now as our preferred means.

It would make a good Pactoberfest challenge, which others could get involved in, as something that isn't too huge, if its done peicemeal but could possibly offer a good amount of value to the pact ecosystem.

@YOU54F
Copy link
Member

YOU54F commented Jul 7, 2023

Do we actually want to retain the api, rather than just exposing the bin stubs, as part of the migration to pact-js-cli (so the api functions come with it)

Also, post including rust with the core, we migrated from downloading the ruby standalone package at user install time, to switching to pre-baking into the published package. I assume we would want them pre-baked as part of pact-js-cli. I've only just spun it up now, so haven't checked

@YOU54F
Copy link
Member

YOU54F commented Jul 7, 2023

So I've got a PR against pact-js-cli which introduces a few things in line with this change, details in the PR.

pact-foundation/pact-js-cli#1

there is an open question relating to versioning of the package on its initial release

the standalone packages will be included in the npm package, so this comment/question isn't relevant any more

Also, post including rust with the core, we migrated from downloading the ruby standalone package at user install time, to switching to pre-baking into the published package. I assume we would want them pre-baked as part of pact-js-cli. I've only just spun it up now, so haven't checked

@mefellows
Copy link
Member

Question - do we want the API to live on in pact-cli package? or is that going to have it's final hurrah in this release of pact-js-core?

I think pact-core (this repo) should just be the FFI bindings and the CLI include both the typed interface around the CLI and the CLI itself.

@TimothyJones
Copy link
Contributor Author

^ I agree, the API for the CLI belongs in the CLI

@YOU54F
Copy link
Member

YOU54F commented Jul 8, 2023

so it doesn’t look like the api made it over

assuming we are thinking the same thing
CanDeploy/PublishPacts etc

@TimothyJones
Copy link
Contributor Author

That’s right, this PR doesn’t include the API.

@TimothyJones
Copy link
Contributor Author

Er, sorry-I mean the cli package doesn’t include the API (nor does this PR- it removes it)

@YOU54F
Copy link
Member

YOU54F commented Jul 11, 2023

Thanks for the clarification Tim!

I've raised an issue for releasing the cli to npm and just some q's about version/changelog

pact-foundation/pact-js-cli#2

just to track that progress

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.

None yet

3 participants