-
Notifications
You must be signed in to change notification settings - Fork 218
feat: dependent resource in the condition instead of resource #1690
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
Conversation
Please retry analysis of this Pull-Request directly on SonarCloud. |
Will add migration guide. |
* @param context the current reconciliation {@link Context} | ||
* @return {@code true} if the condition holds, {@code false} otherwise | ||
*/ | ||
boolean isMet(P primary, R secondary, Context<P> context); | ||
boolean isMet(DependentResource<R, P> dependentResource, P primary, Context<P> context); |
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.
Maybe keep the order that was there before (i.e. primary as first param)?
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.
Hmm, don't, have a strong opinion on that. Changed this because the condition applies primarily to a dependent resource, so that's why it is on first place (kinda leading or priority)
We should add an example where the dependent resource is actually used because, as it stands, this change only seems to complicate things while not actually addressing any particular issue… |
when this will be merged: #1688 |
Kudos, SonarCloud Quality Gate passed! |
No description provided.