feat: forward service env to service targets and make it round-trip safe#9018
Open
JeffreyCA wants to merge 1 commit into
Open
feat: forward service env to service targets and make it round-trip safe#9018JeffreyCA wants to merge 1 commit into
JeffreyCA wants to merge 1 commit into
Conversation
Correctness:
- Propagate the caller's resolver into nested ServiceConfig conversions in
the ProjectConfig -> proto mapping so project-level payloads
(ProjectService.Get, event Project payloads) expand service env values
instead of blanking them; wrap nested conversion errors with the service
name so one malformed service is diagnosable.
- Escape '$' and '\' when reverse-mapping extension-supplied env values
(new osutil.NewLiteralExpandableString) so literal values survive later
envsubst expansion instead of being reinterpreted as templates.
- Preserve unchanged ${VAR} env templates in azure.yaml when a service
config round-trips through ProjectService.AddService, so read-modify-write
extension flows no longer bake expanded values into the persisted file.
- Resolve the environment lazily per conversion in external framework and
service target providers (shared serviceConfigToProto helper) instead of
freezing a possibly-nil env at first registration; log env-load failures
instead of silently expanding everything to empty strings.
- Use the session environment (honoring -e/--environment) in
ProjectService.Get and GetResolvedServices instead of the persisted
default environment, matching the framework/service-target/event paths.
- Log the previously swallowed conversion error in RequiredExternalTools.
- Add the environment field to the extension scaffolding models.proto so
scaffolded extensions can consume it.
Cleanup:
- Remove the now-unused lazyEnvManager dependency from projectService.
- Consolidate the duplicated framework service registration tests into one
table-driven test; env expansion behavior is asserted in pkg/project.
- Use Expand with error assertions and $-containing fixtures in mapper
tests so escaping regressions fail visibly.
- Document expansion and write-back semantics in the extension framework
docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR builds on #8936 (forward service
envto extensions, tracked by #9008) and hardens the service-levelenvforwarding so it is complete, environment-aware, and safe to round-trip back intoazure.yaml.Context
#8936 forwarded expanded service
envto framework providers, but left gaps that made it incomplete and unsafe for extensions that persist config.ServiceConfigwith noenvironment.Get/GetResolvedServicesresolved against the default environment (ignoring-e), and nested conversions dropped the resolver, soGetProjectexpanded serviceenvto empty strings.pa$$wordcould be reinterpreted, and anAddServiceround-trip baked resolved values intoazure.yaml, discarding the author's${VAR}templates.Changes
Service target providers now receive resolved env, via a shared
serviceConfigToProto(lazyEnv, serviceConfig)helper used by both external provider types.Environment resolution is now uniform and honors
-e.Get/GetResolvedServicesresolve through a singleenvResolver()backed by the sessionlazyEnv, the resolver is propagated into nested service conversions, and a missing environment expands to empty strings instead of failing.Round-trips back into
azure.yamlare safe. Incoming values are wrapped withosutil.NewLiteralExpandableString(escaping$and\), andAddServicepreserves the original${VAR}template for every env entry whose expanded value is unchanged.Docs explain reading versus authoring.
ServiceConfig.environmentcarries expanded values for providers to read, while raw${VAR}templates are authored through theenvpath of the service config RPCs (GetServiceConfigSection/SetServiceConfigSection, or theValuevariants).Behavior at a glance
serviceConfig.EnvironmentGetProjectservice-levelenvGet/GetResolvedServicesenvironment-e-eAddServiceround-trip of an unchangedenventry${VAR}template preserved$(for examplepa$$word)Using it as an extension author
A provider reads already-expanded values from
serviceConfig.Environment:Given
env: { API_ENDPOINT: ${SERVICE_API_ENDPOINT} }, a read-modify-write throughServiceConfigkeeps the template intact (unchanged entries are not baked to their resolved values):To author or edit a raw
${VAR}template, use the service config RPCs instead ofAddService, sinceServiceConfig.environmentonly carries expanded values (escape a literal$as$$):Testing
$.osutil.NewLiteralExpandableStringcoverage for template syntax,$, backslashes, and UNC paths.AddService.