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

Make mediation invitation parameter idempotent #1413

Conversation

chumbert
Copy link
Contributor

@chumbert chumbert commented Sep 14, 2021

To ease Aca-py's deployment in scenarios where command line arguments
cannot be changed easily, make --mediator-invitation and
--mediator-connections-invite available to the provision command.

provision stores the mediator invite to be used by the agent (effectively "provisioning" it).
start uses the stored invite. Once used, it does not establish connection again.
If the invitation changes, the stored value is replaced by the new one, the default mediator is cleared and the new invitation
is used to establish connection.

Boy-scouting: I split the mediation invite tests apart from the other ones for the sake of easier comprehension/code management.

Signed-off-by: Clément Humbert <clement.humbert@sicpa.com>
To ease Aca-py's deployment in scenarios where command line arguments
cannot be changed easily, make `--mediatior-invitation` and
`--mediator-connections-invite` available to the `provision` command.

`provision` stores the mediator invite to be used by `start` command. If
the invitation changed, `start` updates the stored invite and uses the
new one.

Signed-off-by: Clément Humbert <clement.humbert@sicpa.com>
Whenever the conductor needs to setup a new mediation connection (eg:
the invitation url provided by the command line arguments has changed),
clear the old default mediator settings prior to setting up the new
connection and default mediator.

Signed-off-by: Clément Humbert <clement.humbert@sicpa.com>
Signed-off-by: Clément Humbert <clement.humbert@sicpa.com>
@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2021

Codecov Report

Merging #1413 (b19debe) into main (d1af978) will decrease coverage by 0.01%.
The diff coverage is 89.42%.

@@            Coverage Diff             @@
##             main    #1413      +/-   ##
==========================================
- Coverage   95.32%   95.30%   -0.02%     
==========================================
  Files         482      483       +1     
  Lines       29154    29237      +83     
==========================================
+ Hits        27791    27865      +74     
- Misses       1363     1372       +9     

dbluhm
dbluhm previously approved these changes Sep 15, 2021
Copy link
Contributor

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

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

LGTM!

print("Attempting to connect to mediator...")
del mgr
except Exception as e:
print(e)
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra print statement

@andrewwhitehead
Copy link
Contributor

I don't really understand what the goal is here. It seems like it's meant to store the mediation invitation if you add the argument to the provision command, but the conductor startup process is still looking for the mediation.invite setting instead of the stored invitation. It seems like the argument would need to be specified again in order to be accepted at startup?

@chumbert
Copy link
Contributor Author

I don't really understand what the goal is here. It seems like it's meant to store the mediation invitation if you add the argument to the provision command, but the conductor startup process is still looking for the mediation.invite setting instead of the stored invitation. It seems like the argument would need to be specified again in order to be accepted at startup?

Our issue is that in an automated scalable environment, new aca-py instances are spinned up with the same arguments as the original instance, thus retaining the mediation.invite setting. We want to avoid these new instances creating new connections to the mediator if one is already established, hence the need for idempotency.

However, I agree with your point that the conductor should not necessarily require the mediation.invite to be specified if one was provisioned and is already stored. I'll make sure this can be either/or.


return updated_record

async def get_mediation_invite_record(
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this method should store and return the provided invitation, if any. At the moment it does not return the provided invitation if there is no stored invitation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I missed a case. Good catch

Mediation invite can be:
* Specified at provision time and repeated at start time without
  creating more connections that necessary
* Specified at provision and not repeated afterwards

Stored invite is updated if a new invite is provided through arguments.

Signed-off-by: Clément Humbert <clement.humbert@sicpa.com>
@chumbert chumbert force-pushed the idempotent-mediator-invitation-url branch from 52903d8 to 59a62b1 Compare September 21, 2021 06:42
@andrewwhitehead andrewwhitehead merged commit 1f60a8a into openwallet-foundation:main Sep 21, 2021
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.

4 participants