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

Include children when targeting resources with children. #7605

Merged
merged 6 commits into from
Jul 28, 2021

Conversation

komalali
Copy link
Member

@komalali komalali commented Jul 22, 2021

Description

Prior to this change, when a component was targeted without including all of its children in the list of targets, it would result in an error like below:

Diagnostics:
  infrastructure:k8s-istio-operator (web-dev):
    error: Cannot delete parent resource 'urn:pulumi:istio-operator::infrastructure::infrastructure:k8s-istio-operator::web-dev' without also deleting child 'urn:pulumi:istio-operator::infrastructure::infrastructure:k8s-istio-operator$kubernetes:yaml:ConfigGroup::web-dev'

However, when a component is targeted, it is logical that the user intended to also target the component's children, since the component itself is a wrapper for all of its children. This PR changes the behavior to include children when a component is targeted instead of erroring.

Note: This changes the default behavior of --target to include a component's children. Should this be behind a flag (should we be including this in --target-dependents)? Do we want some way to preserve the old behavior?

Fixes #4032

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

@komalali komalali force-pushed the komalali/target-deps branch 2 times, most recently from 5b6afb5 to bd0348c Compare July 23, 2021 05:37
@komalali komalali changed the title [WIP] - Expand --target-dependents to include children. [WIP] - Include children when targeting components. Jul 23, 2021
@komalali komalali force-pushed the komalali/target-deps branch 3 times, most recently from 9ea9bb0 to fb57f06 Compare July 23, 2021 17:52
@komalali komalali changed the title [WIP] - Include children when targeting components. Include children when targeting components. Jul 23, 2021
@komalali komalali marked this pull request as ready for review July 23, 2021 18:37
@komalali
Copy link
Member Author

@pgavlin I know we discussed solving this at the CLI level - is there any drawback to fixing it here?

@komalali
Copy link
Member Author

Tagged @lukehoban and @infin8x to consider the change from default behavior and how we want to handle that.

Copy link
Member

@pgavlin pgavlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent tests!

One nit + one suggestion re: finding the transitive children of a resource.

pkg/resource/deploy/step_generator.go Show resolved Hide resolved
pkg/resource/deploy/step_generator.go Outdated Show resolved Hide resolved
pkg/resource/deploy/step_generator.go Outdated Show resolved Hide resolved
jancespivo pushed a commit to jancespivo/pulumi that referenced this pull request Aug 4, 2021
@komalali komalali changed the title Include children when targeting components. Include children when targeting resources with children. Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failure previewing a subset of stack's tree.
2 participants