aries-cloudagent-python icon indicating copy to clipboard operation
aries-cloudagent-python copied to clipboard

refactor: remove connection protocol

Open dbluhm opened this issue 1 year ago • 9 comments

This PR Removes the deprecated connections protocol.

In addition to the message handlers and routes, this also means:

  • CLI flags relating to connections protocol removed.
  • References to specific messages in base connection manager were removed; there will need to be some working around this in the plugin to continue to support the protocol but I think it's good and necessary to at least rip it out for now here.
  • Introduction protocol used to use Connections invitations. I did a very rough-and-ready conversion to OOB invites.
  • Connections management routes (e.g. querying connections) were moved to aries_cloudagent.connections.routes (from aries_cloudagent.protocols.connections.v1_0.routes).

I am expecting this to not be fully functional yet so opening as a draft for now.

dbluhm avatar Aug 19 '24 21:08 dbluhm

@dbluhm it was mentioned on the call this was moved to a plugin. Is the code to enable using that plugin included in this pr?

PatStLouis avatar Aug 20 '24 16:08 PatStLouis

It’s in the plugin repo: https://github.com/hyperledger/aries-acapy-plugins/pull/925

swcurran avatar Aug 20 '24 17:08 swcurran

@swcurran thank you, I was referring towards the "code required to make this plugin work" which was highlighted in the meeting.

The functionality lost/broken features outlined here, will this require code from within the aca-py code base or it can all be managed in the external plugin?

PatStLouis avatar Aug 20 '24 17:08 PatStLouis

Doh…sorry about that. Good point. And per my comment in the meeting — is it viable to simply document how to use the plugin? I suppose not being able to use the artifacts directly (e.g., having to change the Python code) might be a bit too painful.

swcurran avatar Aug 20 '24 17:08 swcurran

With the most recent updates on this branch and to the corresponding plugin, I have successfully completed a connection using the plugged in connection protocol. There are some failing tests I will address but I will go ahead and mark this as ready for review.

dbluhm avatar Sep 19 '24 15:09 dbluhm

Quality Gate Failed Quality Gate failed

Failed conditions
52.8% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

sonarqubecloud[bot] avatar Sep 23 '24 16:09 sonarqubecloud[bot]

Cool. Let’s include in our discussion tomorrow the version number if we remove this. 1.1.0?

Should the SonarCloud analysis be addressed before merging?

swcurran avatar Sep 23 '24 17:09 swcurran

I'm just wondering if maybe a scenario test that installs the plugin and creates a v1 connection might be a good idea.

We can do that though this is basically what's happening in the integration test of the plugin itself. So I'm not sure how valuable it is for it to be tested in both places.

We'll need good docs for when it gets released.

Indeed, I have not adequately addressed this need yet.

Should the SonarCloud analysis be addressed before merging?

I am not too concerned personally. The most complaints are coming from code that was just moved but detected as new in the connection routes (moved from the protocols.connections.routes to connections.routes).

dbluhm avatar Sep 23 '24 19:09 dbluhm

Quality Gate Failed Quality Gate failed

Failed conditions
37 Security Hotspots
4.7% Duplication on New Code (required ≤ 3%)
D Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

sonarqubecloud[bot] avatar Oct 10 '24 15:10 sonarqubecloud[bot]

Fun list of merge conflicts to address. I'll work on getting this PR updated but then we ought to merge ASAP to prevent this from happening again :sweat_smile:

dbluhm avatar Nov 12 '24 17:11 dbluhm

This PR has been refreshed after growing stale. It is ready for review and merge whenever we're ready with it.

dbluhm avatar Jan 22 '25 20:01 dbluhm

Quality Gate Failed Quality Gate failed

Failed conditions
54.8% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

sonarqubecloud[bot] avatar Jan 22 '25 20:01 sonarqubecloud[bot]

@jamshale @loneil and other @openwallet-foundation/acapy-admins and @openwallet-foundation/acapy-committers -- please review and let's get this merged before it goes stale again. I love the "new code not tested" since this is a PR about removal... I assume we can ignore that, but would like a dev to review/comment.

Thanks

swcurran avatar Jan 22 '25 21:01 swcurran

Sorry — I junped the gun on this one. I also should have waited until the connections plugin was in place. I think there will be some falling out with this one before it is complete.

swcurran avatar Jan 23 '25 18:01 swcurran

Sorry — I junped the gun on this one. I also should have waited until the connections plugin was in place. I think there will be some falling out with this one before it is complete.

Also this is now breaking some OATH tests tat are still using connection protocol ...

ianco avatar Jan 23 '25 19:01 ianco

Yes — expected. Part of the fix/cleanup will be adding the plugin. We’ll just live with it for now — I’ll try to deal with it later. @ianco — much more concerned about getting the intermittent fix resolved in OATH and integration tests here.

swcurran avatar Jan 23 '25 19:01 swcurran