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.

Sign up for GitHub

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

Jump to bottom

feat(thirdparty): add tp-api-svc and round out thirdparty sub-chart #454

Merged
merged 26 commits into from
Oct 7, 2021
Merged

feat(thirdparty): add tp-api-svc and round out thirdparty sub-chart #454

merged 26 commits into from
Oct 7, 2021

Conversation

lewisdaly
Copy link
Contributor

Closes mojaloop/project#2450

Adds the final service for 3P-API support in Mojaloop! Also includes some docs and examples for how to install everything which I've tested against the oss-lab-beta cluster.

What's next:

  • tidy up the participant-list-svc config: it's a little nasty
  • add a new thirdparty-simulator chart to support testing of the 3P-API

@lewisdaly lewisdaly mentioned this pull request Sep 27, 2021
Closed
13 tasks
@lewisdaly lewisdaly marked this pull request as ready for review September 29, 2021 08:46
mdebarros
mdebarros requested changes Oct 5, 2021
View reviewed changes
Copy link
Member

@mdebarros mdebarros left a comment

Choose a reason for hiding this comment

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

Hey @lewisdaly, made a couple of change suggestions and comments that will need to be addressed.

Let me know if you have any questions.

package.sh Outdated
@@ -47,6 +47,7 @@ else
mojaloop
kube-system/ntpd/
ml-operator
thirdparty
Copy link
Member

Choose a reason for hiding this comment

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

Note: Ordering is important here!

As thirdparty is a dependency on mojaloop, please move it above mojaloop....otherwise you will have an incomplete dependency tree.

thirdparty/README.md
# skip these steps if you want to deploy them yourself
cd chart-auth-svc
kubectl apply -f ./example_dependencies.yaml
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 not add something like -n {mojaloop-namespace} to the command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just an example... it just takes the current namespace

Copy link
Member

Choose a reason for hiding this comment

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

I believe individuals will take it verbatim. But the important part is that it should be deployed within the same Mojaloop namespace right, and if its not perhaps include some steps on what should be addressed.

thirdparty/chart-auth-svc/values.yaml Outdated Show resolved Hide resolved
thirdparty/chart-consent-oracle/Chart.yaml Outdated
@@ -1,14 +1,14 @@
apiVersion: v1
description: consent-oracle chart for Mojaloop Thirdparty Overlay Services
name: consent-oracle
version: 0.1.0
version: 1.0.0
Copy link
Member

Choose a reason for hiding this comment

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

Should charts be 1.0 if the app itself is not at least v1.0?

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 dunno, not too fussed about this so I'll change it back

thirdparty/chart-tp-api-svc/participant-list.svc.yaml Outdated
# we can't specify inline TTK configs from within this values file.
##

COMPUTED VALUES:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
COMPUTED VALUES:
COMPUTED VALUES: {}

or should this be []?

OR is there an indent issue here?

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 think I can just remove that line

thirdparty/values.yaml Outdated
# Location of central ledger to lookup endpoints for
"ENDPOINT_SERVICE_URL": "http://mojaloop-centralledger-service:80",
# Location of service that implements `GET /SERVICES/{TYPE}` call
"PARTICIPANT_LIST_SERVICE_URL": "http://tp-svc.sandbox.mojaloop.io",
Copy link
Member

Choose a reason for hiding this comment

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

Same issue as above with regard to the missing $release_name prefix.

thirdparty/values.yaml
"PARTICIPANT_ID": "centralauth",
"REDIS": {
"PORT": 6379,
"HOST": "auth-svc-redis-svc",
Copy link
Member

Choose a reason for hiding this comment

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

Same issue as above with regard to the missing $release_name prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not in this case - the auth-svc-redis-svc service name comes from the example dependencies that aren't deployed as a part of mojaloop

thirdparty/values.yaml Outdated
"SHARED": {
"PEER_ENDPOINT": "thirdparty-api-adapter:3008",
"ALS_ENDPOINT": "mojaloop-account-lookup-service",
"QUOTES_ENDPOINT": "0.0.0.0:3002",
Copy link
Member

Choose a reason for hiding this comment

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

Same issue as above with regard to the missing $release_name prefix.

thirdparty/values.yaml Outdated
"PEER_ENDPOINT": "thirdparty-api-adapter:3008",
"ALS_ENDPOINT": "mojaloop-account-lookup-service",
"QUOTES_ENDPOINT": "0.0.0.0:3002",
"TRANSFERS_ENDPOINT": "0.0.0.0:3000",
Copy link
Member

Choose a reason for hiding this comment

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

Same issue as above with regard to the missing $release_name prefix.

thirdparty/values.yaml Outdated
"ALS_ENDPOINT": "mojaloop-account-lookup-service",
"QUOTES_ENDPOINT": "0.0.0.0:3002",
"TRANSFERS_ENDPOINT": "0.0.0.0:3000",
"BULK_TRANSFERS_ENDPOINT": "",
Copy link
Member

Choose a reason for hiding this comment

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

Same issue as above with regard to the missing $release_name prefix.

@lewisdaly
Copy link
Contributor Author

Thanks @mdebarros. I'm working on a fix to the tp-api-svc (mojaloop/thirdparty-api-svc#62) so we can remove the need for a ttk simulator to fill the place of the participant list service and fix most of the issues you identified.

@lewisdaly lewisdaly requested a review from mdebarros October 7, 2021 03:08
@lewisdaly
Copy link
Contributor Author

@mdebarros good to go again.

mdebarros
mdebarros requested changes Oct 7, 2021
View reviewed changes
Copy link
Member

@mdebarros mdebarros left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Super minor typo changes, and just some clarification questions (i.e. did you mean to remove those).

mojaloop/values.yaml Outdated Show resolved Hide resolved
mojaloop/values.yaml Outdated
@@ -5311,9 +5303,12 @@ thirdparty:
"generateTimeout": 30000
},
# Location of central ledger to lookup endpoints for
"ENDPOINT_SERVICE_URL": "http://mojaloop-centralledger-service:80",
# Location of service that implements `GET /SERVICES/{TYPE}` call
"PARTICIPANT_LIST_SERVICE_URL": "http://tp-svc.sandbox.mojaloop.io",
Copy link
Member

Choose a reason for hiding this comment

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

What happened to this guy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR: mojaloop/thirdparty-api-svc#62 removed the need for it.

thirdparty/chart-auth-svc/values.yaml Outdated Show resolved Hide resolved
thirdparty/chart-auth-svc/values.yaml
"ALS_ENDPOINT": "mojaloop-account-lookup-service",
"QUOTES_ENDPOINT": "0.0.0.0:3002",
"TRANSFERS_ENDPOINT": "0.0.0.0:3000",
"BULK_TRANSFERS_ENDPOINT": "",
Copy link
Member

Choose a reason for hiding this comment

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

What happened to the rest of these end-points?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need them - they're covered in "PEER_ENDPOINT"

thirdparty/chart-tp-api-svc/README.md Outdated Show resolved Hide resolved
thirdparty/chart-tp-api-svc/values.yaml Outdated
@@ -53,9 +51,12 @@ config:
"generateTimeout": 30000
},
# Location of central ledger to lookup endpoints for
"ENDPOINT_SERVICE_URL": "http://mojaloop-centralledger-service:80",
# Location of service that implements `GET /SERVICES/{TYPE}` call
"PARTICIPANT_LIST_SERVICE_URL": "http://tp-svc.sandbox.mojaloop.io",
Copy link
Member

Choose a reason for hiding this comment

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

PARTICIPANT_LIST_SERVICE_URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR: mojaloop/thirdparty-api-svc#62 removed the need for it.

thirdparty/values.yaml Outdated
"ALS_ENDPOINT": "mojaloop-account-lookup-service",
"QUOTES_ENDPOINT": "0.0.0.0:3002",
"TRANSFERS_ENDPOINT": "0.0.0.0:3000",
"BULK_TRANSFERS_ENDPOINT": "",
Copy link
Member

Choose a reason for hiding this comment

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

Just confirming again that these are correctly being removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep

thirdparty/values.yaml Outdated
@@ -298,9 +290,12 @@ tp-api-svc:
"generateTimeout": 30000
},
# Location of central ledger to lookup endpoints for
"ENDPOINT_SERVICE_URL": "http://mojaloop-centralledger-service:80",
# Location of service that implements `GET /SERVICES/{TYPE}` call
"PARTICIPANT_LIST_SERVICE_URL": "http://tp-svc.sandbox.mojaloop.io",
Copy link
Member

Choose a reason for hiding this comment

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

PARTICIPANT_LIST_SERVICE_URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR: mojaloop/thirdparty-api-svc#62 removed the need for it.

@mojaloopci mojaloopci merged commit 74c1ce4 into mojaloop:master Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdebarros mdebarros mdebarros approved these changes

@elnyry-sam-k elnyry-sam-k Awaiting requested review from elnyry-sam-k

@vijayg10 vijayg10 Awaiting requested review from vijayg10

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[helm] - Add 3ppi services helm charts
3 participants
@lewisdaly @mdebarros @mojaloopci