-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[ENH] Update sample app install env vars injection #5930
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
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
|
Enhance CLI sample app env var injection and cloud credential reuse This PR refines the sample app installer to pre-populate environment variables with defaults defined in Key Changes• Extended Affected Areas• rust/cli/src/commands/install.rs This summary was automatically generated by @propel-code-bot |
| app_config | ||
| .required_env_variables | ||
| .iter() | ||
| .for_each(|env_var| { | ||
| env_variables.0.insert(env_var.name.clone(), "".to_string()); | ||
| let value = env_var.default.clone().unwrap_or_default(); | ||
| env_variables.0.insert(env_var.name.clone(), value); | ||
| }); | ||
|
|
||
| app_config | ||
| .optional_env_variables | ||
| .iter() | ||
| .for_each(|env_var| { | ||
| env_variables.0.insert(env_var.name.clone(), "".to_string()); | ||
| let value = env_var.default.clone().unwrap_or_default(); | ||
| env_variables.0.insert(env_var.name.clone(), value); | ||
| }); |
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.
[Maintainability] To avoid repeating logic, you can chain the iterators for required_env_variables and optional_env_variables together and apply the for_each loop once. This is consistent with how chain is used in the new app_requests_env function.
| app_config | |
| .required_env_variables | |
| .iter() | |
| .for_each(|env_var| { | |
| env_variables.0.insert(env_var.name.clone(), "".to_string()); | |
| let value = env_var.default.clone().unwrap_or_default(); | |
| env_variables.0.insert(env_var.name.clone(), value); | |
| }); | |
| app_config | |
| .optional_env_variables | |
| .iter() | |
| .for_each(|env_var| { | |
| env_variables.0.insert(env_var.name.clone(), "".to_string()); | |
| let value = env_var.default.clone().unwrap_or_default(); | |
| env_variables.0.insert(env_var.name.clone(), value); | |
| }); | |
| app_config | |
| .required_env_variables | |
| .iter() | |
| .chain(app_config.optional_env_variables.iter()) | |
| .for_each(|env_var| { | |
| let value = env_var.default.clone().unwrap_or_default(); | |
| env_variables.0.insert(env_var.name.clone(), value); | |
| }); |
Context for Agents
To avoid repeating logic, you can chain the iterators for `required_env_variables` and `optional_env_variables` together and apply the `for_each` loop once. This is consistent with how `chain` is used in the new `app_requests_env` function.
```suggestion
app_config
.required_env_variables
.iter()
.chain(app_config.optional_env_variables.iter())
.for_each(|env_var| {
let value = env_var.default.clone().unwrap_or_default();
env_variables.0.insert(env_var.name.clone(), value);
});
```
File: rust/cli/src/commands/install.rs
Line: 472| #[clap(long, conflicts_with_all = ["name"])] | ||
| list: bool, | ||
| #[clap(long, hide = true)] | ||
| dev: Option<String>, |
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 didn't mean to completely remove this one
Allows injecting a cloud token and lets sample apps provide env var values.