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

[v2] Set config all at once #718

Merged
merged 1 commit into from
Oct 17, 2024
Merged

[v2] Set config all at once #718

merged 1 commit into from
Oct 17, 2024

Conversation

blampe
Copy link
Contributor

@blampe blampe commented Oct 16, 2024

We currently issue one SetAllConfig RPC for each user-specified config. This is slow but it has important correctness guarantees:

  1. The order we apply config matters -- if the user specifies foo: foo followed by foo: bar, the net result must always be foo: bar.
  2. The Pulumi CLI (and therefore Automation API) only allows specifying --path on an all-or-nothing basis. This is bad for us because we potentially have a blend of path and non-path keys.

Ideally we would be able to supply all of our configs to the automation API in a single call, and in the case where all of our config keys are path-like (or all are not path-like) we actually can do that because we no longer have limitation (2).

This PR makes that possible in the general case by transforming our config keys in a way that allows us to treat them as if they are all path-like.

In particular:

  • The agent's SetAllConfig handler is modified to take a list of configs instead of a map in order to preserve config order. The top-level path param is also removed and handled on a per-key basis.
  • While resolving configs, we escape any non-path keys so subsequent path parsing treats them as verbatim. For example foo.bar gets escaped as ["foo.bar"].
  • We can then supply all of our keys at once to Automation API with Path: true.
  • If there are no configs to set then the operator doesn't invoke SetAllConfig.

Fixes #650

@blampe blampe added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Oct 16, 2024
Copy link

codecov bot commented Oct 16, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 22 lines in your changes missing coverage. Please review.

Project coverage is 53.10%. Comparing base (7883699) to head (6647054).
Report is 3 commits behind head on v2.

Files with missing lines Patch % Lines
agent/pkg/server/server.go 73.33% 4 Missing and 4 partials ⚠️
operator/internal/controller/auto/utils.go 0.00% 7 Missing ⚠️
...r/internal/controller/auto/workspace_controller.go 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##               v2     #718      +/-   ##
==========================================
+ Coverage   53.00%   53.10%   +0.09%     
==========================================
  Files          29       29              
  Lines        3081     3096      +15     
==========================================
+ Hits         1633     1644      +11     
- Misses       1267     1269       +2     
- Partials      181      183       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@blampe
Copy link
Contributor Author

blampe commented Oct 17, 2024

Confirmed config still propagates as expected. Of course the StacK API is currently just a map with no path information, so all of the path optimizations are only relevant for the Workspace API. However it does give users the ability -- although somewhat clunky -- to specify somewhat structured (path: true) config via the workspaceTemplate.

@blampe blampe merged commit 6b9e71f into v2 Oct 17, 2024
7 checks passed
@blampe blampe deleted the blampe/config branch October 17, 2024 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/no-changelog-required This issue doesn't require a CHANGELOG update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants