flowey: add first-class config support for flow nodes#3160
flowey: add first-class config support for flow nodes#3160jstarks wants to merge 10 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces first-class “config” plumbing for Flowey nodes, separating configuration (versions/flags/local paths) from action requests and removing the same_across_all_reqs() pattern across the Flowey ecosystem.
Changes:
- Add
flowey_config!,FlowNodeWithConfig,new_flow_node_with_config!, andNodeCtx::config()plus resolver/runtime/pipeline-db plumbing to merge and deliver config independently of requests. - Convert multiple existing nodes and job wiring to use typed config structs (Option fields + map merges) instead of config-as-request variants.
- Regenerate CI pipeline YAMLs to match updated Flowey execution indices and serialization.
Reviewed changes
Copilot reviewed 48 out of 48 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| flowey/flowey_lib_hvlite/src/resolve_openvmm_deps.rs | Move version/local-path selection into merged node config. |
| flowey/flowey_lib_hvlite/src/resolve_openhcl_kernel_package.rs | Convert kernel package version/local-path selection to node config. |
| flowey/flowey_lib_hvlite/src/install_vmm_tests_deps.rs | Replace selection request with config-based selection. |
| flowey/flowey_lib_hvlite/src/install_git_credential_manager.rs | Convert feature toggles to config options. |
| flowey/flowey_lib_hvlite/src/init_openvmm_cargo_config_deny_warnings.rs | Move deny-warnings toggle into node config. |
| flowey/flowey_lib_hvlite/src/git_checkout_openvmm_repo.rs | Replace SetRepoId request with config-based repo id. |
| flowey/flowey_lib_hvlite/src/download_uefi_mu_msvm.rs | Convert version/local path to merged config. |
| flowey/flowey_lib_hvlite/src/download_openvmm_vmm_tests_artifacts.rs | Move local-only prompt/policy/cache defaults into config. |
| flowey/flowey_lib_hvlite/src/cfg_rustup_version.rs | Switch from request-based to config-based Rust toolchain pinning. |
| flowey/flowey_lib_hvlite/src/_jobs/local_build_and_run_nextest_vmm_tests.rs | Update job wiring to set node config instead of config-requests. |
| flowey/flowey_lib_hvlite/src/_jobs/consume_and_test_nextest_vmm_tests_archive.rs | Update job wiring to set node config instead of config-requests. |
| flowey/flowey_lib_hvlite/src/_jobs/cfg_versions.rs | Centralize tool versions via ctx.config(...); add .NET version constant and dotnet installer node import. |
| flowey/flowey_lib_hvlite/src/_jobs/cfg_hvlite_reposource.rs | Switch git checkout + openvmm repo selection to config. |
| flowey/flowey_lib_hvlite/src/_jobs/cfg_gh_azure_login.rs | Switch Azure login credentials to config. |
| flowey/flowey_lib_hvlite/src/_jobs/cfg_common.rs | Replace multiple config-like requests with config structs across nodes. |
| flowey/flowey_lib_common/src/use_gh_cli.rs | Add config struct + config-friendly auth enum for gh CLI node. |
| flowey/flowey_lib_common/src/run_cargo_nextest_run.rs | Move default fail-fast / terminate behavior into config. |
| flowey/flowey_lib_common/src/resolve_protoc.rs | Convert version/local-path selection into config + updated validation messages. |
| flowey/flowey_lib_common/src/install_rust.rs | Replace AutoInstall/IgnoreVersion/Version requests with config fields. |
| flowey/flowey_lib_common/src/install_nodejs.rs | Convert version + auto-install settings to config. |
| flowey/flowey_lib_common/src/install_git.rs | Convert local auto-install toggle to config. |
| flowey/flowey_lib_common/src/install_dotnet_cli.rs | Introduce config for dotnet channel + auto-install and adopt FlowNodeWithConfig. |
| flowey/flowey_lib_common/src/install_dist_pkg.rs | Convert interactive/skip-update toggles to config. |
| flowey/flowey_lib_common/src/install_azure_cli.rs | Convert version/auto-install to config. |
| flowey/flowey_lib_common/src/git_checkout.rs | Add config for local clone policy; plumb through local/ado/github emit paths. |
| flowey/flowey_lib_common/src/gh_task_azure_login.rs | Convert credentials into config. |
| flowey/flowey_lib_common/src/download_mdbook_mermaid.rs | Convert mdbook-mermaid version to config. |
| flowey/flowey_lib_common/src/download_mdbook_admonish.rs | Convert mdbook-admonish version to config. |
| flowey/flowey_lib_common/src/download_mdbook.rs | Convert mdbook version to config. |
| flowey/flowey_lib_common/src/download_gh_cli.rs | Convert gh CLI version to config. |
| flowey/flowey_lib_common/src/download_cargo_nextest.rs | Convert cargo-nextest version to config. |
| flowey/flowey_lib_common/src/download_cargo_fuzz.rs | Convert cargo-fuzz version to config. |
| flowey/flowey_lib_common/src/download_azcopy.rs | Convert azcopy version to config. |
| flowey/flowey_lib_common/src/cfg_cargo_common_flags.rs | Convert locked/verbose settings to config. |
| flowey/flowey_core/src/node.rs | Core API: ConfigVar, config merge traits, config routing (NodeCtx::config), and FlowNodeWithConfig integration. |
| flowey/flowey_cli/src/pipeline_resolver/viz.rs | Adjust resolver callsites for new stage1_dag return shape (includes config_db). |
| flowey/flowey_cli/src/pipeline_resolver/github_yaml/mod.rs | Persist job-level configs into pipeline static DB and pass through YAML resolver path. |
| flowey/flowey_cli/src/pipeline_resolver/direct_run.rs | Adjust resolver callsite for new stage1_dag return shape. |
| flowey/flowey_cli/src/pipeline_resolver/ado_yaml.rs | Persist job-level configs into pipeline static DB and pass through ADO YAML resolver path. |
| flowey/flowey_cli/src/flow_resolver/stage1_dag.rs | Plumb config events, track outstanding config, emit config_db, and pass config bytes into node emit. |
| flowey/flowey_cli/src/cli/exec_snippet.rs | Load config bytes from pipeline static DB and pass into node emit. |
| flowey/flowey_cli/src/cli/debug/interrogate.rs | Update node.emit invocation for new signature (currently always passes empty config list). |
| ci-flowey/openvmm-pr.yaml | Regenerated Flowey-based ADO pipeline YAML (line/idx updates). |
| .github/workflows/openvmm-pr.yaml | Regenerated GitHub Actions workflow (line/idx updates). |
| .github/workflows/openvmm-pr-release.yaml | Regenerated GitHub Actions workflow (line/idx updates). |
| .github/workflows/openvmm-docs-pr.yaml | Regenerated GitHub Actions workflow (line/idx updates). |
| .github/workflows/openvmm-docs-ci.yaml | Regenerated GitHub Actions workflow (line/idx updates). |
| .github/workflows/openvmm-ci.yaml | Regenerated GitHub Actions workflow (line/idx updates). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
flowey/flowey_lib_common/src/use_gh_cli.rs:104
use_gh_clicurrently errors onmissing config: authbefore checking whether anyGetrequests were made. With config now able to activate nodes, this makes it easy to fail a flow even whenuse_gh_cliisn't actually used (or when only other nodes are being exercised). Consider handling theget_reqs.is_empty()case first and only requiringauthwhenGetis requested (optionally still claiming any provided token as unused).
flowey/flowey_lib_hvlite/src/download_openvmm_vmm_tests_artifacts.rs
Outdated
Show resolved
Hide resolved
flowey/flowey_lib_hvlite/src/download_openvmm_vmm_tests_artifacts.rs
Outdated
Show resolved
Hide resolved
|
needs guide update |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 53 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
flowey/flowey_lib_common/src/gh_task_azure_login.rs:1
- With
credentials: Option<ConfigVar<OpenIDConnect>>, the config merge requiresConfigVar<OpenIDConnect>: PartialEq. As implemented,ConfigVar<T>currently requiresT: PartialEq + Eq + Clone + Serialize + DeserializeOwned, butOpenIDConnectin this file does not show those derives/impls. This is likely to fail to compile. Fix by deriving/implementing the required traits onOpenIDConnect(at leastClone,PartialEq,Eq, plus serde traits if required byConfigVar), or by looseningConfigVar<T>bounds soTdoesn’t need them for identity-based equality.
// Copyright (c) Microsoft Corporation.
flowey/flowey_lib_common/src/install_dotnet_cli.rs:1
- This changes behavior from the previous defaulting logic (previously the node defaulted to a .NET channel when no version was specified) to making
versionmandatory at runtime. If callers relied on the prior default, this becomes a breaking change. Consider restoring the old default behavior (e.g., default to\"8.0\"whenconfig.versionisNone), or make the required-ness explicit by ensuring all call sites always set it and updating the node docs/comments accordingly.
Flowey nodes that accept "config" values--versions, feature flags, auto-install toggles--currently encode them as Request enum variants and use same_across_all_reqs() to assert that every caller agrees on the value. This is verbose, error-prone, and mixes configuration with action requests. Introduce a dedicated config channel so that config is declared as a struct, merged automatically across callers, and delivered to emit() separately from requests. This replaces the same_across_all_reqs pattern with typed, Option-field config structs and a flowey_config! macro. The core additions are the FlowNodeWithConfig trait, new_flow_node_with_config! macro, ConfigVar<T> (a ReadVar wrapper with identity-based equality for merge), and ImportCtx::config(). The resolver, pipeline serializers, and exec_snippet runtime are updated to plumb config alongside requests. All existing nodes that used same_across_all_reqs are converted to the new config pattern, along with their callers. CI pipeline YAMLs are regenerated.
- Add guard fn to IntoConfig trait (matches IntoRequest pattern) - Update Guide nodes.md to document FlowNodeWithConfig and flowey_config! - Replace stage1_dag tuple return with named Stage1DagOutput struct - Replace resolve_flow_as_*_yaml_steps tuple return with ResolvedFlowSteps - Migrate CustomCacheDir fully into config, remove request variant - Add PipelineJob::config() for pipeline-level config support - Plumb seed_configs through all resolvers to stage1_dag - Remove unnecessary ConfigVar->ReadVar conversions in nodes - Make GhCliAuth private (only used internally after config refactor)
flowey/flowey_core/src/node.rs
Outdated
| /// The config type generated by [`flowey_config!`](crate::flowey_config). All fields are | ||
| /// `Option<T>` — the node decides which are required vs optional. |
There was a problem hiding this comment.
FlowNodeWithConfig’s associated Config type is documented as “All fields are Option<T>”, but the new flowey_config! pattern also supports non-Option mergeable fields (e.g. BTreeMap<..>), and this PR already uses maps in several config structs. Please update this doc comment to reflect the actual supported field shapes.
| /// The config type generated by [`flowey_config!`](crate::flowey_config). All fields are | |
| /// `Option<T>` — the node decides which are required vs optional. | |
| /// The config type generated by [`flowey_config!`](crate::flowey_config). | |
| /// | |
| /// Scalar fields are typically wrapped in `Option<T>`, and the node decides which | |
| /// options are treated as required vs optional. Configs may also include | |
| /// non-`Option` mergeable fields (for example, maps) that are combined according | |
| /// to the [`ConfigMerge`] implementation. |
| - **Requests** (`flowey_request!`): actions that produce outputs, e.g. `GetBinary(WriteVar<PathBuf>)`. Multiple callers can each submit requests, and the node fulfills all of them. | ||
|
|
||
| ```admonish tip | ||
| If a value needs `same_across_all_reqs` validation, it should probably be a config field instead. The `flowey_config!` macro provides this validation automatically during merge. |
There was a problem hiding this comment.
Are we still using same_across_all_reqs anywhere? Could we remove it? Should we?
There was a problem hiding this comment.
We should remove it if we can. I found one more user but it was a bad pattern so I fixed it.
I'll have to check the closed-source repo.
Flowey nodes that accept "config" values--versions, feature flags, auto-install toggles--currently encode them as Request enum variants and use same_across_all_reqs() to assert that every caller agrees on the value. This is verbose, error-prone, and mixes configuration with action requests.
Introduce first-class config types: config is declared as a struct, is merged automatically across callers, and is delivered to emit() separately from requests. This replaces the same_across_all_reqs pattern with typed, Option-field config structs and a flowey_config! macro.
The core additions are the FlowNodeWithConfig trait, new_flow_node_with_config! macro, ConfigVar (a ReadVar wrapper with identity-based equality for merge), and ImportCtx::config(). The resolver, pipeline serializers, and exec_snippet runtime are updated to plumb config alongside requests.
All existing nodes that used same_across_all_reqs are converted to the new config pattern, along with their callers. CI pipeline YAMLs are regenerated.