[Resolves #1187] Add configuration for inheritance strategies for list and dict configs

okcleary commented 9 months ago

Add configuration for inheritance strategies for list and dict configs.

This resolves #1187 by allowing selecting a dict_merge strategy at a stack group/stack level, while remaining backwards compatible with existing stacks by retaining the default of child_wins.

This does not however address the example from the original issue of including tags of dependencies (or dependents?), as I don't believe this fits with the existing model (or makes sense as a use case).

This PR additionally adds the same support to all list and dict based configs.

The config for inheritance strategy itself is also inheritable, but not configurable.

From the original issue example:

Example: Assume Sceptre project A, B and C with dependency C -> B

root StackConfig config/config.yaml

stack_tags_inheritance: dict_merge
stack_tags:
  - country: US

config/prod/A.yaml

stack_tags:
  - city: anaheim    # stack A tags are 'city:anaheim', 'country:US'

config/prod/C.yaml

dependencies:
  - prod/B.yaml
stack_tags:
  - county: collin   # stack C tags are 'county:collin', 'country:US'

config/prod/B.yaml

stack_tags:         # In contrast to original issue example, dependencies do not impact inherited values
  - city: boston    # stack B tags are 'city:boston', 'country:US'

PR Checklist

[x] Wrote a good commit message & description [see guide below].
[x] Commit message starts with [Resolve #issue-number].
[x] Added/Updated unit tests.
[ ] Added/Updated integration tests (if applicable).
[x] All unit tests (poetry run tox) are passing.
[x] Used the same coding conventions as the rest of the project.
[x] The new code passes pre-commit validations (poetry run pre-commit run --all-files).
[x] The PR relates to only one subject with a clear title. and description in grammatically correct, complete sentences.

Approver/Reviewer Checklist

[ ] Before merge squash related commits.

Other Information

Guide to writing a good commit

alex-harvey-z3q commented 8 months ago

@okcleary Thanks for your contribution. This looks like a great feature and we have discussed in the Slack channel that I am currently running your branch locally to make sure nothing breaks for me. Could you help me to review this PR by explaining to me how we can be sure that the change is backwards-compatible and will not break anything for others?

okcleary commented 8 months ago

@okcleary Thanks for your contribution. This looks like a great feature and we have discussed in the Slack channel that I am currently running your branch locally to make sure nothing breaks for me. Could you help me to review this PR by explaining to me how we can be sure that the change is backwards-compatible and will not break anything for others?

Thanks! There are two points to consider here for the backwards-compatibility of this change.

The first one is the decision to not change any of the existing merge strategies - using a different strategy is optin by the user, and the existing default strategy of each key is untouched by this PR.

The second point is the change to merging the stackgroup config, seen here:

https://github.com/Sceptre/sceptre/pull/1445/files#diff-f46036fa4ab9b5ddee081163999046ae153ec9a37458c96fc312a3e8d6cf53f4L356

Currently only the only key merged is dependencies which is done explicitly. This PR changes it so the merge behaves in the same way as the stack merges - considering all keys that could be mergeable. This change is backwards compatible because all the other keys besides dependencies have child_wins as the default strategy so the behaviour is the same as before.

okcleary commented 8 months ago

Thanks @zaro0508, I've updated with the suggested documentation changes.

Sceptre / sceptre