feat(operator): add support for pod template #5369

jsenko · 2024-10-18T14:09:57Z

No description provided.

jsenko · 2024-10-18T14:10:31Z

@andreaTP Please review, I'll take a look at your PR soon.

operator/controller/src/main/java/io/apicurio/registry/operator/utils/Mapper.java

operator/controller/src/main/java/io/apicurio/registry/operator/utils/PodTemplateSpecUtils.java

operator/controller/src/test/java/io/apicurio/registry/operator/unit/MergePTSTest.java

operator/controller/src/test/resources/podtemplatespec-test-cases.yaml

jsenko · 2024-10-23T12:21:26Z

@andreaTP I've changed the approach to implementing the PTS support. Let me know what you think.

jsenko · 2024-10-23T12:33:47Z

I can close and reopen the PR if you think this is too cluttered.

andreaTP

@jsenko I overall like where this is going, let's try to continue here if you are not too bothered by the comments.

I think we can improve by separating the "merge things" logic from the getDefaultXXXDeployment.

i.e.:

getDefaultXXDeployment -> constructs in code the default base without any check, from scratch
mergeXYZ -> contains all the logic for merging the default base with the user-defined values

The distinction will also make clear the intentions and semantic decisions over the merging strategies.

Please, remember that, before merging we should have some sort of documentation about the decisions we are taking when merging stuff.

operator/controller/src/main/java/io/apicurio/registry/operator/resource/ResourceFactory.java

...ntroller/src/main/java/io/apicurio/registry/operator/resource/app/AppDeploymentResource.java

operator/controller/src/main/java/io/apicurio/registry/operator/utils/Utils.java

andreaTP · 2024-10-24T09:12:31Z

operator/controller/src/main/java/io/apicurio/registry/operator/utils/Utils.java

+     *
+     * @param target must not be null
+     */
+    public static <T, K> void mergeNotOverride(List<T> target, List<T> source, Function<T, K> extractKey) {


I'm convinced this can be done in a simpler way, let me know if I should take a stab at it 👍

Can you create a followup PR for this please? This PR does not have to be perfect, is should be correct. I'll be happy to review your PR.

I'm ok with this, agreed.

Follow up PR created: jsenko#1055

operator/controller/src/test/java/io/apicurio/registry/operator/it/PTSITTest.java

andreaTP · 2024-10-24T09:15:51Z

operator/controller/src/test/java/io/apicurio/registry/operator/unit/PodTemplateSpecTest.java

+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+public class PodTemplateSpecTest {


Nice job here! Clear and well organized!

jsenko · 2024-10-24T13:24:03Z

@jsenko I overall like where this is going, let's try to continue here if you are not too bothered by the comments.

I think we can improve by separating the "merge things" logic from the getDefaultXXXDeployment.

i.e.:
* `getDefaultXXDeployment` -> constructs in code the default base without any check, from scratch

* `mergeXYZ` -> contains all the logic for merging the default base with the user-defined values
The distinction will also make clear the intentions and semantic decisions over the merging strategies.

Please, remember that, before merging we should have some sort of documentation about the decisions we are taking when merging stuff.

Updated.

jsenko · 2024-10-24T13:28:49Z

Please, remember that, before merging we should have some sort of documentation about the decisions we are taking when merging stuff.

We do not have docs ported yet. What kind of documentation do you have in mind? The merge logic is stupidly simple at the moment and can be documented later.

andreaTP · 2024-10-24T13:50:39Z

What kind of documentation do you have in mind?

Even a section in the Readme of the sub-module is fine by me.

The merge logic is stupidly simple

It's "stupidly simple" to you that have written the code, knows where to look for finding information, knows how to use the IDE etc. etc. I think we can improve in inclusiveness here and allow anyone looking at this PR to access a description in plain English of what we are trying to achieve.

All in all, it can be easy or complicated, but there is no documented decision about the tradeoffs we took:

merging containers based on names
merging ports based on the name of the port
rewriting completely probes when defined in PodTemplate
etc.

I want to be able to read a spec, reason about it, and verify the implementation.
Writing the code and extracting the rationale after the fact may incur in documented bugs and misinterpretations.

Once we have proper documentation in place we can move the text around.

operator/controller/src/main/java/io/apicurio/registry/operator/resource/ResourceFactory.java

…ts and limits separately

jsenko · 2024-10-29T11:29:56Z

@andreaTP I've added information to the README.

andreaTP · 2024-10-29T11:37:40Z

operator/controller/README.md

+To allow the testing of Ingresses when using minikube, run:
+  ```shell
+  minikube addons enable ingress
+  minikube tunnel


side note: this mechanism seems to be not enough on my machine, but I have not narrowed it down.

hmm it works for me. Maybe try with the latest version?

andreaTP · 2024-10-29T11:38:59Z

operator/controller/README.md

+
+If your cluster does not have an accessible ingress host, you can skip them using `test.operator.ingress-skip=true` (**not recommended**).
+
+## PodTemplateSpec merging


Thanks 👍

andreaTP · 2024-10-29T11:42:28Z

operator/controller/src/main/java/io/apicurio/registry/operator/resource/ResourceFactory.java

+        } else {
+            r.getSpec().setTemplate(new PodTemplateSpec());
+        }
+        mergeDeploymentPodTemplateSpec(


I believe that the most idiomatic place to do this merging is in XXXDeploymentResource so that we have a clear distinction:

what comes as default -> comes from RegistryFactory

user defined customizations -> reconciled in the DeploymentResource (i.e. before setting the env var for the datasources)

But we can follow up in a separate PR.

andreaTP

LGTM, good to go, thanks for bearing with me @jsenko

jsenko requested a review from andreaTP October 18, 2024 14:09

apicurio-bot bot added the triage/needs-triage label Oct 18, 2024

jsenko removed the triage/needs-triage label Oct 21, 2024

andreaTP reviewed Oct 21, 2024

View reviewed changes

jsenko force-pushed the operator-7 branch 2 times, most recently from 4b143e1 to b65a6b5 Compare October 23, 2024 12:33

jsenko force-pushed the operator-7 branch from b65a6b5 to 7a18bdb Compare October 23, 2024 14:47

andreaTP requested changes Oct 24, 2024

View reviewed changes

andreaTP reviewed Oct 25, 2024

View reviewed changes

operator/controller/src/main/java/io/apicurio/registry/operator/resource/ResourceFactory.java Show resolved Hide resolved

jsenko and others added 6 commits October 29, 2024 10:29

feat(operator): add support for pod template

88dab56

refactor(operator): code improvements

cbaf753

refactor(operator): change approach to PTS support

90bf005

refactor(operator): address code review

8bab7d4

a more concise implementation of merge not override

8e0df46

chore: spotless apply

a5bb212

jsenko force-pushed the operator-7 branch from 76988ab to a5bb212 Compare October 29, 2024 09:42

jsenko added 2 commits October 29, 2024 11:23

feat(operator): handle resources as a whole in PTS, instead of reques…

5418ce8

…ts and limits separately

docs(operator): initial documentation of PTS merging

02b533e

andreaTP reviewed Oct 29, 2024

View reviewed changes

andreaTP approved these changes Oct 29, 2024

View reviewed changes

jsenko merged commit 0490d15 into Apicurio:main Oct 29, 2024
19 checks passed

jsenko deleted the operator-7 branch October 29, 2024 12:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(operator): add support for pod template #5369

feat(operator): add support for pod template #5369

jsenko commented Oct 18, 2024

jsenko commented Oct 18, 2024

jsenko commented Oct 23, 2024

jsenko commented Oct 23, 2024

andreaTP left a comment

andreaTP Oct 24, 2024

jsenko Oct 24, 2024 •

edited

Loading

andreaTP Oct 24, 2024

andreaTP Oct 28, 2024

andreaTP Oct 24, 2024

jsenko Oct 24, 2024

jsenko commented Oct 24, 2024

jsenko commented Oct 24, 2024 •

edited

Loading

andreaTP commented Oct 24, 2024

jsenko commented Oct 29, 2024

andreaTP Oct 29, 2024

jsenko Oct 29, 2024

andreaTP Oct 29, 2024

andreaTP Oct 29, 2024

andreaTP left a comment


		If your cluster does not have an accessible ingress host, you can skip them using `test.operator.ingress-skip=true` (not recommended).

		## PodTemplateSpec merging

feat(operator): add support for pod template #5369

feat(operator): add support for pod template #5369

Conversation

jsenko commented Oct 18, 2024

jsenko commented Oct 18, 2024

jsenko commented Oct 23, 2024

jsenko commented Oct 23, 2024

andreaTP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsenko Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsenko commented Oct 24, 2024

jsenko commented Oct 24, 2024 • edited Loading

andreaTP commented Oct 24, 2024

jsenko commented Oct 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaTP left a comment

Choose a reason for hiding this comment

jsenko Oct 24, 2024 •

edited

Loading

jsenko commented Oct 24, 2024 •

edited

Loading