-
Notifications
You must be signed in to change notification settings - Fork 42
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
Fix #304 - Operator driven service discovery API #283
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.
I like this initial (big) step. Therefore, can we please have one package? It's uncommon in golang to have that many packages for this implementation. Especially in the scope of api
package. This is a convention for the CRD types, might be confusing to introduce this here.
What I recommend is to create one single controllers/discovery
package and in there, we create our files with meaningful prefixes such as parser_types.go
. Also, we can implement it by adopting lowercase letters in the func/types and exposing just the discovery engine like: discovery.NewHandler(workflow).Do()
. Just an example.
The other modules/packages don't care about unexposed data structures that we need to do the discovery.
Good suggestions, I'll apply all. Many thanks Ricardo. |
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.
Great work! I really like it! Just a few points if you don't mind. I will refrain from adding more since it's a draft yet. 😅
1033e1a
to
6476298
Compare
6476298
to
d3ed1e0
Compare
@ricardozanini Service discovery was integrated according to the microprofile catalog properties strategy and working for kubernetes services and pods. The other kubernetes objects I think we can add in a separate PR. |
Do we have an issue for this one? |
No. but can I open, the only are the commit comments reffering to KOGITO-9867...., IDK, if you don't mind loosing the history of your review I can open a new PR and use an issue, otherwise I can open an issue and at least link it to this PR. All good now, kie-issue was linked accordingly |
Closes #304 |
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 couple nits, otherwise lgtm
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.
@wmedvede looks good but I really miss unit tests for all the different discovery functions. There are numerous branches in each, for example we have uri_parser_test.go
Could we also do this for other parts?
- Code review suggestions 1, simplify the original packages into the discovery package
- Code review suggestions 2
- Integrate the service discovery API
- Code review suggestions 3, tests added
0c5983c
to
3835f63
Compare
tests where added, this is good enough for now. |
Description of the change:
Motivation for the change:
Checklist
How to backport a pull request to a different branch?
In order to automatically create a backporting pull request please add one or more labels having the following format
backport-<branch-name>
, where<branch-name>
is the name of the branch where the pull request must be backported to (e.g.,backport-7.67.x
to backport the original PR to the7.67.x
branch).Once the original pull request is successfully merged, the automated action will create one backporting pull request per each label (with the previous format) that has been added.
If something goes wrong, the author will be notified and at this point a manual backporting is needed.