-
Notifications
You must be signed in to change notification settings - Fork 4
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
Expose the API key/host/secret as variables for the users #29
Changes from all commits
6a62a14
efdcfde
06e868d
7020d19
5c9adb2
a7c256e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,4 +22,14 @@ callbacks-mapping: | |
inactivity: | ||
service: container | ||
command: ["python", "/usr/local/bin/service-monitor/activity.py"] | ||
timeout: 1 | ||
timeout: 1 | ||
compose-spec: | ||
version: "3.7" | ||
services: | ||
jupyter-math: | ||
image: $${SIMCORE_REGISTRY}/simcore/services/dynamic/jupyter-math:$${SERVICE_VERSION} | ||
environment: | ||
- OSPARC_API_HOST=$${OSPARC_VARIABLE_API_HOST} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very useful feature in the related PR, thanks @pcrespov. If I get this right, if the services specs are adapted correctly, users will not need anymore to create API keys and secrets when they run from inside oSPARC! I am not sure what the conclusion about Small question: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @elisabettai yes, they are. @elisabettai can you please merge these changes and produce a new version of this service and inject in master registry so we can test it there?. thx There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To (auto-magically) publish a new version of this: @wvangeit, can you please run There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @elisabettai could you also deploy it to osparc.io now. |
||
- OSPARC_API_KEY=$${OSPARC_VARIABLE_API_KEY} | ||
- OSPARC_API_SECRET=$${OSPARC_VARIABLE_API_SECRET} | ||
container-http-entrypoint: jupyter-math |
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 does not exist in Osparc codebase ad it will not resolve on all deployments
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.
Do you mean this is because the API server is not available on some deployments? Or just because the variable is not set yet?
@pcrespov in your new PR on simcore, what will happen if there is no API server is available? Will this var be empty/None or so, or will it just not be set?
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.
@GitHK Actually, waiting for you review ;-) ITISFoundation/osparc-simcore#5695
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.
@wvangeit i will then pull it and use it locally.
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.
@wvangeit Good question. In this case, I can see in the code that it simply does not get replaced.
https://github.com/itisfoundation/osparc-simcore/blob/5035fe5cc7a7d7ee9fcab15fd7f8a116a4934cd6/services/director-v2/src/simcore_service_director_v2/modules/dynamic_sidecar/docker_compose_specs.py#L340-L349
Apparently the services will fail upon start if
OSPARC_VARIABLE_API_HOST
is not exposed in osparc. IMO it is a bit extreme since a normal user (i.e. somebody that is not the service owner) will not understand why this service failed to start. If this expression is an an.env
file andOSPARC_VARIABLE_API_HOST
is undefined it thenOSPARC_API_HOST
gets assigned an empty string! Nonetheless @GitHK, hwo is the code owner, has a different view. He will write it down later for you to be aware of itThere 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.
Missed your PR. Got distracted. It's all good now.
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.
I would vote for empty string. Then the user and the python api client can catch this, and notice that the api host is undefined. (in a sense some 'null' instead of empty string would be better, but not sure if there is a stable null equivalent in the world of env vars)
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.
I prefer that
OSPARC_VARIABLE_*
have to be resolvable in order for the service to start. Unlike theOSPARC_VARIABLE_VENDOR_SECRET_*
, which are define din the db, these are provided always by the platform.If you are requesting an
OSPARC_VARIABLE_*
, which does not exist, this is a clear way to signal that you did something wrong.Also I think it's the user's responsibility to make sure that the service stars once they make changes to it. He can always test it in a local deployment before publishing it elsewhere. Unfortunately we don't have a better workflow for this part.
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.