-
Notifications
You must be signed in to change notification settings - Fork 41
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
Break the current operator's configuration into custom and managed properties #367
Conversation
We can add this link to the documentation: https://quarkus.io/guides/config-reference#default-profiles |
@dmartinol can you please verify the e2e failures?
Might be related with these changes. |
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 take a closer look later, but looks ok at a first glance. I let a proposal to avoid including the platform in every creator/mutator.
I see the managed properties do not contain the "converted" discovery properties, so there was not mapping for this configuration:
I'm investigating |
…tCreatorWithPlatform
can you remind me again the documentation repo? |
@ricardozanini apart from some E2E tests that are failing and I cannot replicate on my local environment, unit tests and manual verification seems to succeed. I will update the ADR in the meantime |
Sure thing! https://github.com/apache/incubator-kie-kogito-docs/tree/main/serverlessworkflow/modules/ROOT |
Adding to PR for doc repo |
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.
Looks neat! I just have two questions.
I think you can rebase this PR to take latest Jordi changes to the tests. |
I'm touched by that green flag on the E2E checks 😂 |
@dmartinol looks like we need some love here to fix merge conflicts |
…discovery pattern
and also updated some props, but I need to re-run my manual tests, I'm concerned that latest commits broke anything. |
@dmartinol I agree. Can you open a follow up issue? So I can track it. |
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 have left some minor comments, good job!
@dmartinol @ricardozanini @domhanak some positive feedback for the record. I have executed usecase-oc https://github.com/flows-examples/techpreview2/blob/f39a25e626e217dabf7b69237641ca1e05375c6a/README.md?plain=1#L739 And I can see this results: Managed and normal properties are created I've added a service discovery property manually in the callbackstate-props Then I went to the managed properties and I can see that the service discovery was produced and the corresponding property is in the managed set. The other managed properties are still there. good!
But we can think about this in follow-up PR good job @dmartinol !!!! |
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.
@dmartinol @ricardozanini approving from my side after doing some local testing since my comments are minor suggestions.
Thanks for deep investigation and testing. If you aim to model as env variables only the properties not affected by that issue, this is an option but at the same time we should keep the managed properties file for all of the others, so we'd have 3 managed source of configuration values. |
@dmartinol @ricardozanini @domhanak I have found a small non blocking issue here. When playing to add service discovery properties in the callbackstatetimeouts-prop I define this: After the managed properties calculation was produced in the Reconcile, I can see that the service discovery properties where created, good, but, user.property3 and user.property4 was copied to the managed properties too with a sort of calculated value. |
ok, we can see this in a future step. |
@wmedvede I'd rather keep all managed properties in the CM. One place to look for the managed properties by the operator can facilitate admin tasks. Regarding the bug you've found, this is intentional to override the ones by the user. Otherwise, Quarkus will try to read them and parse. Since Actually, by chance, @dmartinol found a way to remove our
user.property3=<discovered URL>
user.property4=<discovered URL>
user.property3=<desired Kubernetes endpoint>
user.property4=<desired Kubernetes endpoint> We can ditch the jar and live happily. |
@ricardozanini user.property3= Then, regarding the microprofile like generated properties and the removal of that addon, I think we need to investigate a bit, because when we configure knative-serving operations like:
To make them work, if I'm not wrong we need the microprofile addon, need to double check on monday. |
@dmartinol can you please try rebasing? Since we've merged the label PR, I think this line in the e2e is causing troubles:
Your fork still has the old label: https://github.com/dmartinol/incubator-kie-kogito-serverless-operator/blob/SRVLOGIC-195-props/.github/workflows/e2e.yml#L74 |
@wmedvede no worries. The problem is that our addon should consider the property in the default profile, which is not. That's why we had to override in the managed CM. |
@dmartinol once you rebase, CI is green, I'll merge this. |
…operties (apache#367) * Initial commit * integrating comments: introducing ObjectEnsurerWithPlatform and ObjectCreatorWithPlatform * Removing fewe more unneeded references to platform * reviewed workflowProjectHandler. removed user props from managed props * fixed unit tests * workarond for failed discovery options * fixed broken unit tests * fixed mutator for user props * reviewed hashing function * Anticipating deactivation of broken e2e test * integrating comments: removing unneeded comment * adding discovered value to properties whose value mathes the service discovery pattern * removed unused package common_test * Renamed managed props visitor and reviewed description of NewAppPropertyHandler
Closes #381
ConfigMap
to store the managed properties (akaimmutable
)application-PROFILE.properties
contains the properties (PROFILE
is eitherprod
ordev
)application.properties
SonataFlowPlatform
instance, all the creator and mutators have been extended to add aplatform
instance to the input parametersMore tests can be added after we agree on the overall approach.