-
Notifications
You must be signed in to change notification settings - Fork 7
Change bootstrap for sole purposes of enabling testing on your dev instance. #314
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
base: main
Are you sure you want to change the base?
Conversation
export_lines = [ | ||
start_marker + "\n", | ||
"# This section is auto-generated by GiGL/scripts/bootstrap_resource_config.py.\n", | ||
f'export GIGL_TEST_DEFAULT_RESOURCE_CONFIG="{gigl_test_default_resource_config}"\n', | ||
f'export GIGL_PROJECT="{gigl_project}"\n', | ||
f'export GIGL_DOCKER_ARTIFACT_REGISTRY="{gigl_docker_artifact_registry_path}"\n', | ||
end_marker + "\n", | ||
f'export {key}="{value}"\n' for key, value in shell_env_vars.items() | ||
] | ||
export_lines = ( | ||
[ | ||
start_marker + "\n", | ||
"# This section is auto-generated by GiGL/scripts/bootstrap_resource_config.py.\n", | ||
] | ||
+ export_lines | ||
+ [ | ||
end_marker + "\n", | ||
] | ||
) | ||
|
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 whole bit is a bit confusing with the different variables and hard-coded strings and lists.
Can we just use one f-string?
file_loader.load_file( | ||
file_uri_src=file_uri_src, file_uri_dst=resource_config_destination_path | ||
) | ||
shell_env_vars[ | ||
template_config.env_var_to_update |
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.
btw should we log that we created these configs somehwere?
update_shell_config( | ||
shell_config_path=shell_config_path, | ||
shell_env_vars=shell_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.
Hmm, can we keep this update shell config bit as an option? I feel like I would not want to update my shell config when I do this (e.g. when I create some new configs for oss but want to keep the internal config for personal use)
|
||
# Template configurations to process | ||
TEMPLATE_CONFIGS = { | ||
"default": TemplateConfig( |
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.
should we name this tabularized?
template_path=GIGL_ROOT_DIR | ||
/ "deployment" | ||
/ "configs" | ||
/ "e2e_cicd_resource_config.yaml", |
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.
just pointing out some inconsistency in naming re: cicd vs. glt. not sure if this is appropriate / intended
Scope of work done
bootstrap_resource_config.py is currently too flexible, which makes bootstrapping unnecessarily complex. We will simplify it by removing support for arbitrary input/output paths and focusing only on the primary use case: setting up resource configs for local testing. The script’s sole purpose is to help developers quickly configure their machines for unit, integration, and end-to-end tests. For larger projects, configs are set up manually anyway, and this script isn’t being used. Its intended audience is developers onboarding their local environments.
Where is the documentation for this feature?: N/A
Did you add automated tests or write a test plan?
Was used to test the following changes; particularly note the use of env variables:
GiGL/testing/e2e_tests/e2e_tests.yaml
Lines 6 to 21 in db236bc
See the following for the tests that were kicked off:
#301 (comment)
Updated Changelog.md? NO
Ready for code review?: NO