-
Notifications
You must be signed in to change notification settings - Fork 218
feat: workflow Integration with API (dependent annotations, context) #1257
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
e8a1c2f
to
828dc09
Compare
828dc09
to
e1c617f
Compare
@@ -55,7 +55,8 @@ void resetShouldResetAllState() { | |||
shouldBePossibleToOverrideConfigOnce(); | |||
|
|||
ConfigurationServiceProvider.reset(); | |||
assertEquals(ConfigurationServiceProvider.DEFAULT, ConfigurationServiceProvider.getDefault()); | |||
assertNotEquals(ConfigurationServiceProvider.getDefault(), |
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 is wrong. After calling reset, we should get the default ConfigurationService
.
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.
Will comment this in the code, what I wanted to achieve is that reseting it does provide a new instance of the new config, see the explanation below.
@@ -55,7 +55,8 @@ void resetShouldResetAllState() { | |||
shouldBePossibleToOverrideConfigOnce(); | |||
|
|||
ConfigurationServiceProvider.reset(); | |||
assertEquals(ConfigurationServiceProvider.DEFAULT, ConfigurationServiceProvider.getDefault()); | |||
assertNotEquals(ConfigurationServiceProvider.getDefault(), | |||
ConfigurationServiceProvider.createDefault()); |
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.
Also, createDefault
shouldn't be accessible from outside of the ConfigurationServiceProvider
class, imo.
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.
A set it package private so it is testable.
private static ConfigurationService instance; | ||
private static ConfigurationService defaultConfigurationService = DEFAULT; | ||
private static ConfigurationService defaultConfigurationService = createDefault(); |
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.
What's the point of that change? See also comments on the associated test class…
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.
Yes, this solves an issue with the integration tests, basically since it is singleton, it needs to be reseted after the integration tests. But if the DEFAULT is not created again (with this method in reset) , the configurations are for dependents stay in there and causes trouble. This is just for tests.
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.
🤔 I guess the current reset
implementation is indeed wrong…
*/ | ||
@SuppressWarnings("rawtypes") | ||
public class ManagedDependentResourceContext { | ||
public interface ManagedDependentResourceContext { |
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.
What's the point of extracting an interface here when there's only one implementation?
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.
I extracted it for the same reason, since this is a user facing interface, so the setters should not be accessible. Thus is denotes the intention as much as possible.
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/Controller.java
Show resolved
Hide resolved
...torsdk/operator/api/reconciler/dependent/managed/DefaultManagedDependentResourceContext.java
Outdated
Show resolved
Hide resolved
...peratorsdk/operator/processing/dependent/kubernetes/CRUDNoGCKubernetesDependentResource.java
Outdated
Show resolved
Hide resolved
...java/io/javaoperatorsdk/operator/processing/dependent/workflow/builder/DependentBuilder.java
Outdated
Show resolved
Hide resolved
+ dependsOn.stream().map(d -> d.dependentResource.toString()) | ||
.collect(Collectors.joining(", ", "->[", "]")) | ||
+ '}'; | ||
return "DependentResourceNode{" + |
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 version of toString isn't really useful…
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.
Why, basically a reverted this, since you can see easily the parents and the whole structure just by looking on workflow definition. So was debugging this, and searching for issues during the development on lot's of occasions, and this was always sufficient to find the problem.
Kudos, SonarCloud Quality Gate passed! |
No description provided.