-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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 needs to be referencable #23822
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 This does make me wonder what other optional interface checks might be lurking for this, but I don't know of any to suggest specifically, so LGTM to the extent that I follow your line of reasoning. 😁
Looks like those "other optional interface checks" include targeting. I'll look into what's happening there. |
a2cbd40
to
5fabb2a
Compare
Being a |
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,
5fabb2a
to
a6cdfad
Compare
While the NodeDestroyResource type should not be a GraphNodeProviderConsumer, we're going to avoid uncovering more hidden behavior by explicitly skipping provider creation and connections in the provider transformers. This should be removed when more in-depth testing can be done during a major release cycle.
353a4c5
to
4aa8a1c
Compare
Rather than risk discovering any more hidden behaviors associated with this type, add a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that this isn't very pretty, but I'm glad we have a way to do it that minimizes the risk of unintended regressions for now, so that we can defer the more risky change for a time when we are more ready to look at this problem holistically.
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. |
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.
Fixes #23821