-
Notifications
You must be signed in to change notification settings - Fork 1.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
refactor: Introduce OperationContext, to limit the use of reflection. #1433
Conversation
Can one of the admins verify this patch? |
Darn tests are failing... :( |
e06122e
to
9a932ac
Compare
@@ -34,6 +35,7 @@ | |||
import static org.junit.Assert.assertEquals; | |||
import static org.junit.Assert.assertNotNull; | |||
|
|||
@Ignore |
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 is this test ignored?
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.
Ignored that test because it was hunging forever and forgot to deal with it.
+ Wait till resource is deleted from cluster in failing tests
9a932ac
to
66184ba
Compare
*/ | ||
package io.fabric8.kubernetes.client.dsl.base; | ||
|
||
import io.fabric8.kubernetes.api.model.HasMetadata; |
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.
unused import ;-P
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 guy is super picky @iocanel :-D
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.
LoL
return new CustomResourceOperationContext(client, config, plural, namespace, name, apiGroupName, apiGroupVersion, cascading,item, labels, labelsNot, labelsIn, labelsNotIn, fields, resourceVersion, reloadingFromServer, gracePeriodSeconds, crd, type, listType, doneableType); | ||
} | ||
|
||
public CustomResourceOperationContext withRolling(boolean rolling) { |
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 might be wrong here, but can Custom resources can be rollable too 😲
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 don't think so.
Besides these methods doesn't seem to have any effect at all. I'll remove them.
…y to decouple tests from internals.
This pull request, eliminates constructing objects using reflection.
Also simplifies the constructors of operation object by using a single argument.
There are still places that are using reflections, that will be eliminated later in the process).