-
Notifications
You must be signed in to change notification settings - Fork 33
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
Issue 254 | Gateway service #274
Conversation
Number of things is still need to be polished and finished, but it will be separate PRs. Majority of work is done here. Remaining things:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This job is awesome @IceKhan13 . I couldn't review correctly all the code but I started with the django
part. I left some comments with doubts and another ones to discuss. Something in what we can work too is to sync a little bit the projects from repository
and this one.
RUN chmod +x /usr/src/app/entrypoint.sh | ||
|
||
# copy project | ||
COPY . . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This COPY overwrites the edited entrypoint.sh
, doesn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point!
Just to clarify, when creating the gateway provider, the host ( |
No, it is gateway endpoint. And gateway is basically program runner middleware and that is it. Repository service will be living in some other host/port. At the end of the day it is just a matter of configuration. |
Whoops, I should've looked closer at the PR and not tried using the helm install here... will pop back over to docker-compose to poke around. |
@psschwei the idea for the near future is use the gateway like entrypoint (to connect with repository, keycloak, etc), so we will have just one interface (API Gateway) to connect from Middleware client |
When is the Job in the gateway deleted? I haven't looked though changes yet so I might miss it. |
it remains there for references, until user decides to delete it @akihikokuroda . We can put ttl or something like this later on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we talked this is a good first approach. From my part is approved although it will require some improvements that we will cover in future issues / pull requests:
- Sync repository and gateway applications to use same language #307
- Implement versioning for the end-points #308
- Implement validations for NestedProgram fields #310
- Improve Dockerfile configuration for gateway #311
- Verify dj-rest configuration and usage #312
- Add gateway image build into CI #313
- Migrate NestedProgram fields to JSONFields #314
- Configure API projects to use Postgres #315
- Improve test coverage for Gateway end-points #316
- Improve helm configuration introduced with gateway #318
I will create them tomorrow to start handle them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
# incremented each time you make changes to the application. Versions are not expected to | ||
# follow Semantic Versioning. They should reflect the version the application is using. | ||
# It is recommended to use it with quotes. | ||
appVersion: "1.16.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "0.1.0" better?
rayHost: "http://serverless-cluster-kuberay-head-svc:8265" | ||
keycloak: | ||
clientId: "gateway-client" | ||
url: "http://serverless-cluster-keycloak/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is serverless-cluster
your helm install release name? Would you replace it with HELM-RELEASE
realm: "quantumserverless" | ||
clientSecret: "secret" | ||
clientName: "gateway-client" | ||
superuser: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Great work! There are some minor comments but I approve this.
Summary
Gateway service initial works.
Service for managing programs execution, setting limits, etc.
TODOs:
Details and comments
Requests flow
General overview