Ensure that pulumi config cp also copies the environments from the source stack #14847
Conversation
…fig command. Include environments when copying config.
|
PR is now waiting for a maintainer to take action. Note for the maintainer: Commands available:
|
Changelog[uncommitted] (2023-12-14)Bug Fixes
|
| } | ||
|
|
||
| return nil | ||
| return requiresSaving, nil |
There was a problem hiding this comment.
Note that this change had a cascading effect on another place. Though I am not a huge fan of this approach I feel like this is simple and makes this function testable through unit testing.
I am also thinking if I should rename this method from copyEntireConfigMap to something like getMergdConfigMap?
There was a problem hiding this comment.
Returning requiresSaving and making it so callers have to call save doesn't seem the best, but I don't feel strongly about it and agree it's nice that it makes it easier to unit test. I'm fine with it.
I also don't feel strongly about the function name.
This is interesting. The ordering of imported environments is meaningful, as it defines the merge order for the evaluated environment. How were KV pairs handled? |
Ohhh. Hmm that's going to make this tricky. I don't know if we can make assumptions about which should take precedence either, i.e. the source stack's envs vs. the destination's.
In the case of K/V pairs it's simply setting them in the config map. Looking at the YAML marshaller though, I can't tell if it does some sort of ordering implicitly? I feel like I have seen code somewhere that does the key ordering 🤔 EDIT: I just realized you were probably asking about KV handling because there could be key conflicts. We seem to be overwriting them for both single-path and object keys, if I am reading the code correctly. |
|
@pgavlin maybe we should overwrite the destination stack's environment since we can't know how to order the source vs. destination's environments? |
Yeah, I think we should overwrite it. |
|
PR is now waiting for a maintainer to take action. Note for the maintainer: Commands available:
|
|
PR is now waiting for a maintainer to take action. Note for the maintainer: Commands available:
|
|
Adding a quick note that I've updated this PR to overwrite the environments in the destination stack. |
|
/run-acceptance-tests |
|
Looks like there's a lint error @praneetloke https://github.com/pulumi/pulumi/actions/runs/7202401246/job/19620482195 |
|
PR is now waiting for a maintainer to take action. Note for the maintainer: Commands available:
|
|
@komalali thank you. For some reason I didn't get the failure notification. I've fixed the lint error now. |
|
/run-acceptance-tests |
Yeah it's coz the run notifications are buried in the comments for external contributions, like here: #14847 (comment) - it's easy to miss |
|
Ah I see. Yep I definitely overlooked that one. |
There was a problem hiding this comment.
LGTM. Thanks, @praneetloke!
I'll wait to see if @pgavlin has any other feedback, otherwise will merge next week.
| } | ||
|
|
||
| return nil | ||
| return requiresSaving, nil |
There was a problem hiding this comment.
Returning requiresSaving and making it so callers have to call save doesn't seem the best, but I don't feel strongly about it and agree it's nice that it makes it easier to unit test. I'm fine with it.
I also don't feel strongly about the function name.
Yeah, overwriting is probably easier to understand. I worry a little bit about overwriting b/c it's inconsistent with what we do for non-ESC config, where the result is kinda-sorta a merge (just not a deep merge). But I think that the tradeoff in terms of explainability is probably worthwhile. |
Description
This PR updates the copy config command to also copy the environments from the source stack to the destination stack. The list of environments in the destination stack's config will be overwritten.
Fixes #14564
Checklist
make tidyto update any new dependenciesmake lintto verify my code passes the lint checkgofumptmake changelogand committed thechangelog/pending/<file>documenting my change