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

NodeDestroyResource does not need a provider #23696

Merged
merged 1 commit into from
Dec 17, 2019

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Dec 16, 2019

The resource cleanup node does not need a provider. We can't directly
remove the ProvidedBy method, but this node only needs to be eval-able so
we can remove all the NodeAbstractResource methods at once.

Fixes #23671
Fixes #22758

The resource cleanup node does not need a provider. We can't directly
remove the ProvidedBy method, but this node only needs to be eval-able
so we can remove all the NodeAbstractResource methods at once.
@jbardin jbardin requested a review from a team December 16, 2019 22:56
@ghost ghost added the sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK label Dec 16, 2019
Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

adding test coverage + deleting lines of code ❤️😍💕

@jbardin jbardin merged commit 37d2202 into master Dec 17, 2019
@jbardin jbardin deleted the jbardin/orphan-resource-provider branch December 17, 2019 14:09
@apparentlymart apparentlymart added this to the 0.12.19 milestone Jan 7, 2020
jbardin added a commit that referenced this pull request Jan 9, 2020
The change in #23696 removed the NodeAbstractResource methods from the
NodeDestroyResource type, in order to prevent other resource behaviors,
like requesting a provider.

While this node type is not directly referenced, it was implicitly
ordered against the module cleanup by virtue of being a resource node.
The ReferenceTransformer uses the GraphNodeReferenceable and
GraphNodeSubPath interfaces to add nodes to the reference map, so we
need to re-add the relevant methods.

Since there's no good entry point to test this ordering at the moment,
add static checks for the interfaces, and document where these
interfaces are required.
jbardin added a commit that referenced this pull request Jan 9, 2020
The change in #23696 removed the NodeAbstractResource methods from the
NodeDestroyResource type, in order to prevent other resource behaviors,
like requesting a provider.

While this node type is not directly referenced, it was implicitly
ordered against the module cleanup by virtue of being a resource node.
The ReferenceTransformer uses the GraphNodeReferenceable and
GraphNodeSubPath interfaces to add nodes to the reference map, so we
need to re-add the relevant methods.

Since there's no good entry point to test this ordering at the moment,
jbardin added a commit that referenced this pull request Jan 9, 2020
The change in #23696 removed the NodeAbstractResource methods from the
NodeDestroyResource type, in order to prevent other resource behaviors,
like requesting a provider.

While this node type is not directly referenced, it was implicitly
ordered against the module cleanup by virtue of being a resource node.
The ReferenceTransformer uses the GraphNodeReferenceable and
GraphNodeSubPath interfaces to add nodes to the reference map, so we
need to re-add the relevant methods.

Since there's no good entry point to test this ordering at the moment,
jbardin added a commit that referenced this pull request Jan 10, 2020
The change in #23696 removed the NodeAbstractResource methods from the
NodeDestroyResource type, in order to prevent other resource behaviors,
like requesting a provider.

While this node type is not directly referenced, it was implicitly
ordered against the module cleanup by virtue of being a resource node.

Since there's no good entry point to test this ordering at the moment,
@ghost
Copy link

ghost commented Jan 17, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Jan 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
sdkv1 [PRs only] Marks changes that may potentially need to be ported to the plugi nSDK
Projects
None yet
3 participants