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

✨ Auto inject osparc environments to dynamic services #5966

Merged
merged 12 commits into from
Jun 20, 2024

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Jun 19, 2024

What do these changes do?

Injects automatically

environment:
    OSPARC_API_BASE_URL: "$OSPARC_VARIABLE_API_HOST"
    OSPARC_API_KEY: "$OSPARC_VARIABLE_API_KEY"
    OSPARC_API_SECRET: "$OSPARC_VARIABLE_API_SECRET"
    OSPARC_NODE_ID: "$OSPARC_VARIABLE_NODE_ID"
    OSPARC_STUDY_ID: "$OSPARC_VARIABLE_STUDY_UUID"

to all services in a dynamic-service compose. I.e. all dynamic services get automatically access to these environs. NOTE that the key/secrets are customised per user.

@sanderegg @mguidon should we only inject in the "main" service, i.e. the one specified by simcore.service.container-http-entrypoint?

Related issue/s

How to test

  • driving tests services/director-v2/tests/unit/test_modules_osparc_variables.py
  • Manual testing:
    • Open jupyter (or any other dynamic service).
    • printenv | sort | grep OSPARC_ and check that all the env-vars above are defined
      Uploading image.png…

Dev-ops checklist

@pcrespov pcrespov self-assigned this Jun 19, 2024
Copy link

codecov bot commented Jun 19, 2024

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.5%. Comparing base (cafbf96) to head (eb42cc4).
Report is 290 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5966      +/-   ##
=========================================
+ Coverage    84.5%   88.5%    +3.9%     
=========================================
  Files          10    1053    +1043     
  Lines         214   46585   +46371     
  Branches       25     562     +537     
=========================================
+ Hits          181   41248   +41067     
- Misses         23    5214    +5191     
- Partials       10     123     +113     
Flag Coverage Δ
integrationtests 65.5% <91.6%> (?)
unittests 86.1% <87.5%> (+1.5%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...rary/src/models_library/service_settings_labels.py 91.8% <100.0%> (ø)
...v2/modules/dynamic_sidecar/docker_compose_specs.py 97.9% <100.0%> (ø)
...ector_v2/modules/osparc_variables/substitutions.py 74.2% <90.9%> (ø)

... and 1044 files with indirect coverage changes

@pcrespov pcrespov added the a:director-v2 issue related with the director-v2 service label Jun 19, 2024
@pcrespov pcrespov added this to the South Island Iced Tea milestone Jun 19, 2024
@pcrespov pcrespov force-pushed the is5925/osparc-envs-auto branch from f43f0f3 to d4c390a Compare June 19, 2024 13:20
@pcrespov pcrespov changed the title Is5925/osparc envs auto ✨ Auto inject osparc environments to dynamic services by default Jun 19, 2024
@pcrespov pcrespov changed the title ✨ Auto inject osparc environments to dynamic services by default ✨ Auto inject osparc environments to dynamic services Jun 19, 2024
@pcrespov pcrespov marked this pull request as ready for review June 19, 2024 13:24
@pcrespov pcrespov enabled auto-merge (squash) June 19, 2024 13:28
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

that is very nice! thanks!
Now regarding your question, I think for example in s4l the variables are probably needed in the core service which I do not think is the main one. but @mguidon can enlighten us

@pcrespov
Copy link
Member Author

that is very nice! thanks! Now regarding your question, I think for example in s4l the variables are probably needed in the core service which I do not think is the main one. but @mguidon can enlighten us

If that is the case, I do not have the info to know which service is "the core" service

@pcrespov pcrespov requested a review from elisabettai June 20, 2024 07:59
Copy link
Collaborator

@elisabettai elisabettai left a comment

Choose a reason for hiding this comment

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

Very nice, thanks!

Minor thought on the name of the variable OSPARC_NODE_ID: I guess in the code you normally say "node" and this word is mostly used by devs. For user-facing docs, etc... we tend to use more "Service".
But I assume users who will use this envs are enough dev-oriented to understand what that variable represent. 😉

@pcrespov
Copy link
Member Author

Very nice, thanks!

Minor thought on the name of the variable OSPARC_NODE_ID: I guess in the code you normally say "node" and this word is mostly used by devs. For user-facing docs, etc... we tend to use more "Service". But I assume users who will use this envs are enough dev-oriented to understand what that variable represent. 😉

@elisabettai Unfortunately this is already decided and used in the client side. Can you perhaps follow up with a small entry in the manual at some time? Not urgent

Copy link
Contributor

@bisgaard-itis bisgaard-itis left a comment

Choose a reason for hiding this comment

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

Very cool! Thanks a lot

@pcrespov pcrespov force-pushed the is5925/osparc-envs-auto branch from 21a70f9 to d455627 Compare June 20, 2024 15:15
@pcrespov pcrespov force-pushed the is5925/osparc-envs-auto branch from 351e68f to eb42cc4 Compare June 20, 2024 16:10
@pcrespov pcrespov merged commit 68314e4 into ITISFoundation:master Jun 20, 2024
56 checks passed
@pcrespov pcrespov deleted the is5925/osparc-envs-auto branch June 20, 2024 17:23
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Jul 5, 2024
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:director-v2 issue related with the director-v2 service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pass oSparc API necessary environment to all services
5 participants