-
Notifications
You must be signed in to change notification settings - Fork 220
Upgrade to Fabric8's 5.0 client and Quarkus 1.11.0.Final #288
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
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.
The changes to the CustomResource classes are really cool.
This is pure awesomeness, it simplifies usage a lot! Both in SDK and in the code we need to write to use CustomResources. So big 👍 |
@@ -22,7 +22,7 @@ | |||
|
|||
public static final int INITIAL_DELAY = 50; | |||
public static final int PERIOD = 50; | |||
public static final int TESTING_TIME_SLACK = 20; | |||
public static final int TESTING_TIME_SLACK = 40; |
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.
Doesn't look like it's working… 😞
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.
Right... maybe this type of test just isn't a good idea. Probably the Github runner is just really slow. Let me take another look.
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.
They also fail on my system, which, granted is not the fastest around… 😅
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'll rewrite those tests so the timer is mocked, that's the only good solution.
Reverting back to draft pending release of Quarkus 1.11.0.Final so that the upgrade will be made to that version before this PR is merged. |
Should we merge this, @adam-sandor @psycho-ir ? |
Haven’t reviewed this PR yet. Will review it tomorrow |
Quarkus 1.11.0.CR1 is the first version to use Fabric8's client 5.0 so we need to update both at the same time. Note that tests are not expected to pass at this point yet.
The specific CR clients can now be easily retrieved from the fabric8 client using only the CR class.
LGTM |
I gave it a small smoke test (crud on my custom resource) and this PR seems to work. |
Found an issue thanks to @goldmann so reverting to draft untill I can solve it. |
I've tested latest changes with my not-so-trivial codebase and it works great! |
A controller is not cluster-scoped, its associated custom resource type is. As such, this functionality should (and is) controlled by the custom resource type associated with the controller, not the controller itself. If a custom resource implementation doesn't implement Namespaced then it is cluster-scoped.
LGTM |
Quarkus 1.11.0.Final is the first version to use Fabric8's client 5.0 so
we need to update both at the same time.
Overview of the changes:
Doneable
classes have been removed along with all the involved complexityController
annotation has been simplified: thecrdName
field has been removed as that value is computed from the associated custom resource implementationGroup
andVersion
annotations so that they can be identified properly. Optionally, they can also be annotated withKind
(if the name of the implementation class doesn't match the desired kind) andPlural
if the plural version cannot be automatically computed (or the default computed version doesn't match your expectations).CustomResource
class that needs to be extended is now parameterized with spec and status types so you can have an empty default implementation that does what you'd expect. If you don't need a status, usingVoid
for the associated type should work.