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

Enable no-transport mode as startup parameter #2990

Merged
merged 19 commits into from
Jun 24, 2024

Conversation

PatStLouis
Copy link
Contributor

This PR adds a --no-transport startup parameter for making the agent start without requiring the --enpoint, --inbound-transport and --outbound-transport to be set.

The idea was shared during an aca-pug meeting and there was no objection. This can greatly simplify deployment for controllers that do no require didcomm messaging.

Currently it just overrides what is required for startup.

@dbluhm @swcurran @ianco

Signed-off-by: pstlouis <patrick.st-louis@opsecid.ca>
Signed-off-by: pstlouis <patrick.st-louis@opsecid.ca>
@dbluhm
Copy link
Member

dbluhm commented May 28, 2024

Like I mentioned in the ACA-PUG call, I like where this is going. Having the ability to use ACA-Py's credential, DID Resolution, and other capabilities without necessarily using the DIDComm capabilities is really interesting.

I think you're already headed in this direction but if no transport is enabled, it is likely not necessary to register any of the protocols.

PatStLouis and others added 9 commits June 9, 2024 12:46
…in sessions to 0 if no inbound transport manager

Signed-off-by: pstlouis <patrick.st-louis@opsecid.ca>
…-cloudagent-python into pstlouis/no-transport-mode
Signed-off-by: pstlouis <patrick.st-louis@opsecid.ca>
Signed-off-by: pstlouis <patrick.st-louis@opsecid.ca>
Signed-off-by: pstlouis <patrick.st-louis@opsecid.ca>
…-cloudagent-python into pstlouis/no-transport-mode
Signed-off-by: pstlouis <patrick.st-louis@opsecid.ca>
@PatStLouis PatStLouis marked this pull request as ready for review June 16, 2024 23:07
Signed-off-by: pstlouis <patrick.st-louis@opsecid.ca>
aries_cloudagent/core/conductor.py Outdated Show resolved Hide resolved
aries_cloudagent/core/conductor.py Outdated Show resolved Hide resolved
@PatStLouis PatStLouis requested a review from dbluhm June 17, 2024 21:14
dbluhm
dbluhm previously approved these changes Jun 18, 2024
@PatStLouis PatStLouis mentioned this pull request Jun 18, 2024
@dbluhm
Copy link
Member

dbluhm commented Jun 24, 2024

Integration tests are consistently failing. The question is whether that's a result of the changes in this PR or not.

 Failing scenarios:
  features/0586-sign-transaction.feature:124  endorse a schema and cred def transaction, write to the ledger, issue and revoke a credential, manually invoking each endorsement endpoint -- @4.1 
  features/0586-sign-transaction.feature:234  endorse a schema and cred def transaction, write to the ledger, issue and revoke a credential, with auto endorsing workflow -- @1.1 
  features/0586-sign-transaction.feature:245  endorse a schema and cred def transaction, write to the ledger, issue and revoke a credential, with auto endorsing workflow -- @3.1 

It doesn't seem like these changes should impact these scenarios... @jamshale any insights?

@jamshale
Copy link
Contributor

I noticed this but have not dug into it. I'm not sure how this is effecting endorsement but it must be as integration tests are passing on the nightly runs and for other pr's. I can try and see if I can figure anything out.

@PatStLouis
Copy link
Contributor Author

I will have a look, no additional tests were added and no changes are expected if the flag is not added. Maybe there's a bad condition check somewhere.

@jamshale
Copy link
Contributor

I haven't tried to debug it yet but these failing tests are trying to use the /ledger/register-nym endpoint via the register_did method in demo/runners/support/agent.py and getting a 400 error.

Signed-off-by: pstlouis <patrick.st-louis@opsecid.ca>
@PatStLouis
Copy link
Contributor Author

@dbluhm @jamshale I think I found the error, copy paste mistake. I was registering the ledger as a package and not a plugin. Lets see how the test go now

@PatStLouis
Copy link
Contributor Author

Tests passed, updating the branch

Copy link

sonarcloud bot commented Jun 24, 2024

@PatStLouis PatStLouis requested a review from dbluhm June 24, 2024 20:30
@PatStLouis
Copy link
Contributor Author

@dbluhm @jamshale branch updated, all tests are passing. Let me know if you need anything else!

@swcurran swcurran merged commit 66c9c1d into hyperledger:main Jun 24, 2024
8 checks passed
Comment on lines +125 to +126
if not self.settings.get("ledger.disabled"):
plugin_registry.register_plugin("aries_cloudagent.ledger")
Copy link
Contributor

Choose a reason for hiding this comment

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

@jamshale @dbluhm
Just a heads up, this change causes the LedgerAPI to be dropped from the OpenAPI spec that gets generated with bash scripts/generate-open-api-spec, since it uses --no-ledger in its startup args.

Just thought I'd mention. It just means if repo's openapi spec is regenerated, it won't include ledger API. But it should still appear in the swagger.json for an agent that does have ledger configured.

A simple fix is to just revert this line's change. Otherwise, seems like genesis-file / transactions needs to be configured in open-api gen bash script. Seems overkill to do that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can revert this, it's not crucial to what I was trying to achieve here. I'll submit a PR, thanks for flagging this!

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! Picked it up when generating openapi client off nightly build. Thanks! 👍

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

5 participants