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

[APP-2955] Add custom ENV_VAR as Runtime Params #54

Merged
merged 34 commits into from
Sep 5, 2024
Merged

Conversation

amoddhopavkar2
Copy link
Contributor

@amoddhopavkar2 amoddhopavkar2 commented Jul 25, 2024

JIRA Ticket:

Rationale:

  • The point was to support adding custom ENV_VARS as runtime params to dr-apps.
  • The user can pass custom ENV_VARS as:
    • String:
      • --stringEnvVar FOO=BAR, --stringEnvVar API_KEY="RANDOM API KEY"
    • Numeric:
      • --numericEnvVar INT_VAL=3, --numericEnvVar FLOAT_VAL=3.14

Changes in this PR:

  • update_application_source_version: Patches all verified valid runtime params.
  • verify_runtime_env_vars: Checks the runtime param ENV_VARS against the metadata.yaml file.
  • Added a test.

Demo:

APP-2955.mov

@amoddhopavkar2 amoddhopavkar2 changed the title [APP-2955] Add custom ENV_VAR as Runtime Params [APP-2955] Add custom ENV_VAR as Runtime Params Aug 14, 2024
@devexp-slackbot
Copy link

The Needs Review labels were added based on the following file changes.

Team @datarobot/applications (#applications) was assigned because of changes in files:

drapps/create.py
drapps/helpers/custom_app_sources_functions.py
drapps/helpers/runtime_params_functions.py
setup.py
tests/cli/test_create.py
tests/conftest.py

If you think that there are some issues with ownership, please discuss with C&A domain at #core-backend-domain slack channel and create PR to update DRCODEOWNERS\CODEOWNERS file.

Copy link
Contributor

@kideh88 kideh88 left a comment

Choose a reason for hiding this comment

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

One question but otherwise I think its good 👍
Would like to request another review from @ben-cutler-datarobot or @demchukilya to make sure 👀

Comment on lines -185 to -192
# request for uploading project data to source_version
source_version_update_data_matcher = matchers.multipart_matcher(
{'file': ('start-app.sh', entrypoint_script_content)},
data={'baseEnvironmentVersionId': ee_last_version_id, 'filePath': ['start-app.sh']},
)

responses.patch(
f'{api_endpoint_env}/customApplicationSources/{custom_app_source_id}/versions/{custom_app_source_version_id}/',
match=[auth_matcher, source_version_update_data_matcher],
Copy link
Contributor

Choose a reason for hiding this comment

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

What makes this matcher no longer necessary?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's because the matcher for the source version update was not working for multiple calls to the API w/ many different args.

Comment on lines 85 to 89
if runtime_params:
for param in runtime_params:
response = session.patch(url, json={'runtimeParameterValues': param})
handle_dr_response(response)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's better to do as part of main payload instead of separate argument in second request.

Copy link
Collaborator

@demchukilya demchukilya left a comment

Choose a reason for hiding this comment

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

Looks functional, but It's better to fix places noted by me.

Additionally we can have a warning for cases when user try to use runtime parameters with uploading docker image.

drapps/create.py Outdated
Extract the metadata.yaml file from the list of project files..
"""
for file_path, relative_path in project_files:
if relative_path.lower() == 'metadata.yaml':
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we don't need here lower because on backend in DR we do not use it.

drapps/create.py Outdated
custom_app_source_id,
custom_app_source_version_id,
payload,
valid_runtime_params,
Copy link
Collaborator

Choose a reason for hiding this comment

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

In such form, if we have more than 50 files, with each group of files we will have additional request with the same runtime params.

@ben-cutler-datarobot ben-cutler-datarobot self-assigned this Sep 3, 2024
Copy link
Collaborator

@ben-cutler-datarobot ben-cutler-datarobot left a comment

Choose a reason for hiding this comment

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

Tested on staging, and it LGTM
Screenshot 2024-09-04 at 2 21 45 PM
Screenshot 2024-09-04 at 2 20 58 PM

@ben-cutler-datarobot ben-cutler-datarobot merged commit c17022d into main Sep 5, 2024
2 checks passed
@svc-engprod-git1 svc-engprod-git1 deleted the APP-2955 branch September 5, 2024 14:19
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.

5 participants